[PATCH] pci-sysfs: backport fix for 2.6.11.12

[PATCH] pci-sysfs: backport fix for 2.6.11.12

[Michael S. Tsirkin]

Greg, before 2.6.12, pci_write_config in pci-sysfs.c was broken, causing
incorrect data being written to the configuration register,
which in the case of my userspace driver results in system failure.

This has been fixed in 2.6.12-rc5:

http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv2.6%2Ftesting%2Fpatch-2.6.12-rc5.bz2;z=2656

Would you please consider merging the fix for 2.6.11.12 as well?

Alternatively (since there were multiple other changes in pci-sysfs.c), here's
a small patch to fix just this issue.

Thanks,
MST


cast from (signed) char to unsigned int in pci_write_config
causes the result to be sign extended, clobbering high bits in the result.
Thus:

# setpci -s 07:00.0 14.L=E4
# setpci -s 07:00.0 14.L
ffffffe4

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

--- linux-2.6.11-openib/drivers/pci/pci-sysfs.c.bad	2005-05-30 13:45:02.000000000 +0300
+++ linux-2.6.11-openib/drivers/pci/pci-sysfs.c	2005-05-30 13:51:39.000000000 +0300
@@ -161,10 +161,10 @@ pci_write_config(struct kobject *kobj, c
 	}
 
 	while (size > 3) {
-		unsigned int val = buf[off - init_off];
-		val |= (unsigned int) buf[off - init_off + 1] << 8;
-		val |= (unsigned int) buf[off - init_off + 2] << 16;
-		val |= (unsigned int) buf[off - init_off + 3] << 24;
+		unsigned int val = (u8)buf[off - init_off];
+		val |= (unsigned int)(u8)buf[off - init_off + 1] << 8;
+		val |= (unsigned int)(u8)buf[off - init_off + 2] << 16;
+		val |= (unsigned int)(u8)buf[off - init_off + 3] << 24;
 		pci_write_config_dword(dev, off, val);
 		off += 4;
 		size -= 4;

-- 
MST - Michael S. Tsirkin

Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12

[Greg KH]

On Tue, May 31, 2005 at 07:36:19PM +0300, Michael S. Tsirkin wrote:
> Greg, before 2.6.12, pci_write_config in pci-sysfs.c was broken, causing
> incorrect data being written to the configuration register,
> which in the case of my userspace driver results in system failure.
> 
> This has been fixed in 2.6.12-rc5:
> 
> http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv2.6%2Ftesting%2Fpatch-2.6.12-rc5.bz2;z=2656
> 
> Would you please consider merging the fix for 2.6.11.12 as well?

Would you care to split out only the proper part for that fix?  There
are a few different patches in that link above.

> Alternatively (since there were multiple other changes in pci-sysfs.c), here's
> a small patch to fix just this issue.

I don't think this fixes the problem properly.  Can you verify it?
Also, this is only a 64bit issue, right?  What platform are you seeing
this on?

And, any patch that you want to propose for the -stable releases, needs
to be sent to the stable@kernel.org email alias.

thanks,

greg k-h

Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12

[Michael S. Tsirkin]

Quoting r. Greg KH <gregkh@suse.de>:
> Subject: Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12
> 
> On Tue, May 31, 2005 at 07:36:19PM +0300, Michael S. Tsirkin wrote:
> > Greg, before 2.6.12, pci_write_config in pci-sysfs.c was broken, causing
> > incorrect data being written to the configuration register,
> > which in the case of my userspace driver results in system failure.
> > 
> > This has been fixed in 2.6.12-rc5:
> > 
> > http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv2.6%2Ftesting%2Fpatch-2.6.12-rc5.bz2;z=2656
> > 
> > Would you please consider merging the fix for 2.6.11.12 as well?
> 
> Would you care to split out only the proper part for that fix?  There
> are a few different patches in that link above.

Hmm. There's also a new attribute there, that shall wait for 2.6.12.
There was a general cleanup coverting *all* direct uses of char* to
first cast to u8 *, not only in pci_write_config where it triggers
an actual bug. Do you want all that cleanup in?
I can do it, just let me know.

> > Alternatively (since there were multiple other changes in pci-sysfs.c), here's
> > a small patch to fix just this issue.
> 
> I don't think this fixes the problem properly.  Can you verify it?

I did verify it before sending :), my patch below does fix the problem.

> Also, this is only a 64bit issue, right?

No, unsigned int is 32 bit wide on all platforms with gcc,
char is signed and 8 bit wide on all platforms with gcc.
So if you have a char value with a top bit set:

char c = 0xe0;

then the following holds:

((unsigned int)c) == 0xffffffe0

> What platform are you seeing
> this on?

I use Intel x86_64.

> And, any patch that you want to propose for the -stable releases, needs
> to be sent to the stable@kernel.org email alias.
> 
> thanks,
> 
> greg k-h
> 

Okay, since its small I repost here. Let me know whether you want that or
the bigger cleanup backported 2.6.12.

---

cast from (signed) char to unsigned int in pci_write_config causes the result
to be sign extended, clobbering high bits in the result. Thus:

# setpci -s 07:00.0 14.L=E4
# setpci -s 07:00.0 14.L
ffffffe4

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

--- linux-2.6.11-openib/drivers/pci/pci-sysfs.c.bad	2005-05-30 13:45:02.000000000 +0300
+++ linux-2.6.11-openib/drivers/pci/pci-sysfs.c	2005-05-30 13:51:39.000000000 +0300
@@ -161,10 +161,10 @@ pci_write_config(struct kobject *kobj, c
 	}
 
 	while (size > 3) {
-		unsigned int val = buf[off - init_off];
-		val |= (unsigned int) buf[off - init_off + 1] << 8;
-		val |= (unsigned int) buf[off - init_off + 2] << 16;
-		val |= (unsigned int) buf[off - init_off + 3] << 24;
+		unsigned int val = (u8)buf[off - init_off];
+		val |= (unsigned int)(u8)buf[off - init_off + 1] << 8;
+		val |= (unsigned int)(u8)buf[off - init_off + 2] << 16;
+		val |= (unsigned int)(u8)buf[off - init_off + 3] << 24;
 		pci_write_config_dword(dev, off, val);
 		off += 4;
 		size -= 4;
-- 
MST - Michael S. Tsirkin

Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12

[Greg KH]

On Tue, May 31, 2005 at 11:57:29PM +0300, Michael S. Tsirkin wrote:
> Quoting r. Greg KH <gregkh@suse.de>:
> > Subject: Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12
> > 
> > On Tue, May 31, 2005 at 07:36:19PM +0300, Michael S. Tsirkin wrote:
> > > Greg, before 2.6.12, pci_write_config in pci-sysfs.c was broken, causing
> > > incorrect data being written to the configuration register,
> > > which in the case of my userspace driver results in system failure.
> > > 
> > > This has been fixed in 2.6.12-rc5:
> > > 
> > > http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv2.6%2Ftesting%2Fpatch-2.6.12-rc5.bz2;z=2656
> > > 
> > > Would you please consider merging the fix for 2.6.11.12 as well?
> > 
> > Would you care to split out only the proper part for that fix?  There
> > are a few different patches in that link above.
> 
> Hmm. There's also a new attribute there, that shall wait for 2.6.12.

Yes, please remember, different patches can touch the same file :)

> There was a general cleanup coverting *all* direct uses of char* to
> first cast to u8 *, not only in pci_write_config where it triggers
> an actual bug. Do you want all that cleanup in?
> I can do it, just let me know.

Well, I don't think that just applying one part of the whole main patch
is a good idea at all.  Either do it all or not at all.  The original
patch can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4c0619add8c3a8b28e7fae8b15cc7b62de2f8148

Along with the proper description and signed-off-by lines.

> > > Alternatively (since there were multiple other changes in pci-sysfs.c), here's
> > > a small patch to fix just this issue.
> > 
> > I don't think this fixes the problem properly.  Can you verify it?
> 
> I did verify it before sending :), my patch below does fix the problem.

But it doesn't fix the main problem, right?

Honestly, this problem has been around for so long, with no real
complaints from anyone (all the distros already patched this fix a long
time ago), and it's too big for the -stable rules, that I do not think
it should go in.

So no, if you really need this fix, use 2.6.12, or a disto kernel, your
fix is not correct.

thanks,

greg k-h

Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12

[Matthew Wilcox]

On Tue, May 31, 2005 at 11:57:29PM +0300, Michael S. Tsirkin wrote:
> No, unsigned int is 32 bit wide on all platforms with gcc,
> char is signed and 8 bit wide on all platforms with gcc.

Oops, no.  char is signed on x86 and unsigned on ppc.  Other architectures
vary (I believe arm, mips and parisc are also unsigned).

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12

[Michael S. Tsirkin]

Quoting r. Matthew Wilcox <matthew@wil.cx>:
> Subject: Re: [PATCH] pci-sysfs: backport fix for 2.6.11.12
> 
> On Tue, May 31, 2005 at 11:57:29PM +0300, Michael S. Tsirkin wrote:
> > No, unsigned int is 32 bit wide on all platforms with gcc,
> > char is signed and 8 bit wide on all platforms with gcc.
> 
> Oops, no.  char is signed on x86 and unsigned on ppc.  Other architectures
> vary (I believe arm, mips and parisc are also unsigned).

So there's no bug on these platforms :)

-- 
MST - Michael S. Tsirkin