Merge ~rafaeldtinoco/ubuntu/+source/ndctl:lp1811785-bionic into ubuntu/+source/ndctl:ubuntu/bionic-devel

Proposed by Rafael David Tinoco
Status: Rejected
Rejected by: Christian Ehrhardt 
Proposed branch: ~rafaeldtinoco/ubuntu/+source/ndctl:lp1811785-bionic
Merge into: ubuntu/+source/ndctl:ubuntu/bionic-devel
Diff against target: 119 lines (+99/-0)
3 files modified
debian/changelog (+6/-0)
debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch (+92/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Rafael David Tinoco (community) Needs Fixing
Christian Ehrhardt  (community) Needs Fixing
Review via email: mp+375930@code.launchpad.net

Commit message

To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I think the fix itself is good, but a few things need cleanup.

changelog: we usually name the patch before the colon:
so instead
     * ndctl/init-labels: Fix label slot accounting per UEFI 2.7 (LP: #1811785)
maybe:
     * d/p/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch: Fix label slot accounting per UEFI 2.7 (LP: #1811785)

Furthermore the bug should get a proper SRU Template before sponsoring it into -unapproved.

review: Needs Fixing
Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

I'm removing it from canonical-server view based on:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1811785/comments/21

and

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1811785/comments/22

If end-user chooses to continue the SRU, I'll get back to this.

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) :
review: Needs Fixing
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

No activity on this one, setting rejected to get off our overview.
Please revive if needed.

Unmerged commits

319dc82... by Rafael David Tinoco

changelog

a02fb3a... by Rafael David Tinoco

 * ndctl/init-labels: Fix label slot accounting per UEFI 2.7 (LP: #1811785)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index 00774c3..f9e03a6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
1ndctl (61.2-0ubuntu2~18.04.1) bionic; urgency=medium
2
3 * d/p/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch (LP: #1811785)
4
5 -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Sat, 23 Nov 2019 00:44:53 +0000
6
1ndctl (61.2-0ubuntu1~18.04.1) bionic; urgency=medium7ndctl (61.2-0ubuntu1~18.04.1) bionic; urgency=medium
28
3 * Backport to bionic (LP: #1781268)9 * Backport to bionic (LP: #1781268)
diff --git a/debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch b/debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch
4new file mode 10064410new file mode 100644
index 0000000..ea32ae0
--- /dev/null
+++ b/debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch
@@ -0,0 +1,92 @@
1From 9c6aae5635ebe8d0ca56a3ed949eef88639cb1b5 Mon Sep 17 00:00:00 2001
2From: Dan Williams <dan.j.williams@intel.com>
3Date: Mon, 14 Jan 2019 19:51:45 -0800
4Subject: [PATCH] ndctl/init-labels: Fix label slot accounting per UEFI 2.7
5
6Quoting from Linux kernel commit 9e694d9c18dd "libnvdimm, label: change
7nvdimm_num_label_slots per UEFI 2.7":
8
9 sizeof_namespace_index() fails when NVDIMM devices have the minimum
10 1024 bytes label storage area. nvdimm_num_label_slots() returns 3
11 slots while the area is only big enough for 2 slots.
12
13 Change nvdimm_num_label_slots() to calculate a number of label slots
14 according to UEFI 2.7 spec.
15
16Without this fix attempts to initialize labels on a small (1K) label
17area results in the following:
18
19libndctl: sizeof_namespace_index: nmem2: label area (1024) too small to host (128 byte) labels
20libndctl: sizeof_namespace_index: nmem2: label area (1024) too small to host (256 byte) labels
21
22Based on an original patch by Toshi Kani
23Fixes: bdaec95463ca ("ndctl: introduce ndctl_dimm_{validate_labels,init_labels}")
24Reported-by: Sujith Pandel <sujith_pandel@dell.com>
25Link: https://github.com/pmem/ndctl/issues/78
26Signed-off-by: Dan Williams <dan.j.williams@intel.com>
27Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
28---
29 ndctl/lib/dimm.c | 35 +++++++++++++++++++++++++----------
30 1 file changed, 25 insertions(+), 10 deletions(-)
31
32diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
33index b3e032e..108d795 100644
34--- a/ndctl/lib/dimm.c
35+++ b/ndctl/lib/dimm.c
36@@ -66,9 +66,27 @@ static unsigned int sizeof_namespace_label(struct nvdimm_data *ndd)
37 return ndctl_dimm_sizeof_namespace_label(to_dimm(ndd));
38 }
39
40+static size_t __sizeof_namespace_index(u32 nslot)
41+{
42+ return ALIGN(sizeof(struct namespace_index) + DIV_ROUND_UP(nslot, 8),
43+ NSINDEX_ALIGN);
44+}
45+
46+static int __nvdimm_num_label_slots(struct nvdimm_data *ndd,
47+ size_t index_size)
48+{
49+ return (ndd->config_size - index_size * 2) /
50+ sizeof_namespace_label(ndd);
51+}
52+
53 static int nvdimm_num_label_slots(struct nvdimm_data *ndd)
54 {
55- return ndd->config_size / (sizeof_namespace_label(ndd) + 1);
56+ u32 tmp_nslot, n;
57+
58+ tmp_nslot = ndd->config_size / sizeof_namespace_label(ndd);
59+ n = __sizeof_namespace_index(tmp_nslot) / NSINDEX_ALIGN;
60+
61+ return __nvdimm_num_label_slots(ndd, NSINDEX_ALIGN * n);
62 }
63
64 static unsigned int sizeof_namespace_index(struct nvdimm_data *ndd)
65@@ -78,18 +96,15 @@ static unsigned int sizeof_namespace_index(struct nvdimm_data *ndd)
66 struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
67
68 /*
69- * The minimum index space is 512 bytes, with that amount of
70- * index we can describe ~1400 labels which is less than a byte
71- * of overhead per label. Round up to a byte of overhead per
72- * label and determine the size of the index region. Yes, this
73- * starts to waste space at larger config_sizes, but it's
74- * unlikely we'll ever see anything but 128K.
75+ * Per UEFI 2.7, the minimum size of the Label Storage Area is
76+ * large enough to hold 2 index blocks and 2 labels. The
77+ * minimum index block size is 256 bytes, and the minimum label
78+ * size is 256 bytes.
79 */
80 nslot = nvdimm_num_label_slots(ndd);
81 space = ndd->config_size - nslot * sizeof_namespace_label(ndd);
82- size = ALIGN(sizeof(struct namespace_index) + DIV_ROUND_UP(nslot, 8),
83- NSINDEX_ALIGN) * 2;
84- if (size <= space)
85+ size = __sizeof_namespace_index(nslot) * 2;
86+ if (size <= space && nslot >= 2)
87 return size / 2;
88
89 err(ctx, "%s: label area (%ld) too small to host (%d byte) labels\n",
90--
912.20.1
92
diff --git a/debian/patches/series b/debian/patches/series
0new file mode 10064493new file mode 100644
index 0000000..084e496
--- /dev/null
+++ b/debian/patches/series
@@ -0,0 +1 @@
1ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch

Subscribers

People subscribed via source and target branches