| Message ID | 20151124184957.GB27957@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 24 November 2015 at 19:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > Applied as tweaked below to for-linus for v4.4, thanks! As written, > if NUMA_NO_NODE were defined as -2, we would incorrectly accept -1. > Let me know if you disagree with my fix. I don't think the value of NUMA_NO_NODE will (or even has to) ever change, as we're already exporting that value to userland via sysfs. But you're right, the code shouldn't make any assumptions about the concrete value of NUMA_NO_NODE and just handle it as a special symbolic value. > commit 2a35194c5a45fbb9ca1d88bc56804dfb51a75233 > Author: Mathias Krause <minipli@googlemail.com> > Date: Mon Nov 9 20:00:27 2015 +0100 > > PCI: Prevent out of bounds access in numa_node override > > Commit 1266963170f5 ("PCI: Prevent out of bounds access in numa_node > override") missed that the user-provided node could also be negative. > Handle this case as well to avoid out-of-bounds accesses to the > node_states[] array. However, allow the special value -1, i.e. > NUMA_NO_NODE, to be able to set the 'no specific node' configuration. > > [bhelgaas: remove assumption that NUMA_NO_NODE == -1] > Fixes: 1266963170f5 ("PCI: Prevent out of bounds access in numa_node override") > Fixes: 63692df103e9 ("PCI: Allow numa_node override via sysfs") > Signed-off-by: Mathias Krause <minipli@googlemail.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > CC: Sasha Levin <sasha.levin@oracle.com> > CC: Prarit Bhargava <prarit@redhat.com> > CC: stable@vger.kernel.org # v3.19+ > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 9261868..50f4747 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -216,7 +216,12 @@ static ssize_t numa_node_store(struct device *dev, > if (ret) > return ret; > > - if (node >= MAX_NUMNODES || !node_online(node)) > + if (node < 0 || node >= MAX_NUMNODES) { > + if (node != NUMA_NO_NODE) > + return -EINVAL; > + } I would have written something like this: if ((node < 0 && node != NUMA_NO_NODE) || node >= MAX_NUMNODES) return -EINVAL; It's more compact, but your solution is essentially the same, so I'm fine with it. > + > + if (node != NUMA_NO_NODE && !node_online(node)) > return -EINVAL; > > add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); Thanks, Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 24, 2015 at 08:27:04PM +0100, Mathias Krause wrote: > On 24 November 2015 at 19:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > > Applied as tweaked below to for-linus for v4.4, thanks! As written, > > if NUMA_NO_NODE were defined as -2, we would incorrectly accept -1. > > Let me know if you disagree with my fix. > > I don't think the value of NUMA_NO_NODE will (or even has to) ever > change, as we're already exporting that value to userland via sysfs. > But you're right, the code shouldn't make any assumptions about the > concrete value of NUMA_NO_NODE and just handle it as a special > symbolic value. > > > commit 2a35194c5a45fbb9ca1d88bc56804dfb51a75233 > > Author: Mathias Krause <minipli@googlemail.com> > > Date: Mon Nov 9 20:00:27 2015 +0100 > > > > PCI: Prevent out of bounds access in numa_node override > > > > Commit 1266963170f5 ("PCI: Prevent out of bounds access in numa_node > > override") missed that the user-provided node could also be negative. > > Handle this case as well to avoid out-of-bounds accesses to the > > node_states[] array. However, allow the special value -1, i.e. > > NUMA_NO_NODE, to be able to set the 'no specific node' configuration. > > > > [bhelgaas: remove assumption that NUMA_NO_NODE == -1] > > Fixes: 1266963170f5 ("PCI: Prevent out of bounds access in numa_node override") > > Fixes: 63692df103e9 ("PCI: Allow numa_node override via sysfs") > > Signed-off-by: Mathias Krause <minipli@googlemail.com> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > CC: Sasha Levin <sasha.levin@oracle.com> > > CC: Prarit Bhargava <prarit@redhat.com> > > CC: stable@vger.kernel.org # v3.19+ > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index 9261868..50f4747 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -216,7 +216,12 @@ static ssize_t numa_node_store(struct device *dev, > > if (ret) > > return ret; > > > > - if (node >= MAX_NUMNODES || !node_online(node)) > > + if (node < 0 || node >= MAX_NUMNODES) { > > + if (node != NUMA_NO_NODE) > > + return -EINVAL; > > + } > > I would have written something like this: > > if ((node < 0 && node != NUMA_NO_NODE) || node >= MAX_NUMNODES) > return -EINVAL; I adopted that, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 9261868..50f4747 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -216,7 +216,12 @@ static ssize_t numa_node_store(struct device *dev, if (ret) return ret; - if (node >= MAX_NUMNODES || !node_online(node)) + if (node < 0 || node >= MAX_NUMNODES) { + if (node != NUMA_NO_NODE) + return -EINVAL; + } + + if (node != NUMA_NO_NODE && !node_online(node)) return -EINVAL; add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);