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

Subscribers

People subscribed via source and target branches