RE: Questionable code in pci_sal_read

Bjorn Helgaas
2005-01-25T21:22:37Z

On Tue, 2005-01-25 at 12:47 -0800, Luck, Tony wrote:
> >Ah, yes, that looks wrong. Looks like the check for (seg > 255) came
> >from the original pci_sal_read(). The original pci_sal_ext_read() did
> >check for (seg > 65535). My bad.
> >
> >Thanks for catching this.
>
>
> So you (and Matthew Wilcox) are advocating this change?
>
> === arch/ia64/pci/pci.c 1.66 vs edited ==> --- 1.66/arch/ia64/pci/pci.c 2005-01-22 14:42:51 -08:00
> +++ edited/arch/ia64/pci/pci.c 2005-01-25 12:42:49 -08:00
> @@ -71,7 +71,7 @@
> u64 addr, mode, data = 0;
> int result = 0;
>
> - if ((seg > 255) || (bus > 255) || (devfn > 255) || (reg > 4095))
> + if ((seg > 65535) || (bus > 255) || (devfn > 255) || (reg > 4095))
> return -EINVAL;
>
> if ((seg | reg) <= 255) {
>
> "seg", "bus", etc. are all "int" ... should we be extra paranoid
> and check for negative values (or change the definitions to unsigned),
> or is that over the top?

We should definitely change them to unsigned; it's a real problem
that has bitten us already. In fact, I wonder if Andreas was
looking at this code as a result of the bug I opened yesterday ;-)

I'm testing a patch right now, and it includes the "seg > 65535"
change as well.