| Message ID | 1349864358.2386.27.camel@joe-AO722 |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2012-10-10 at 03:19 -0700, Joe Perches wrote: > diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c [] > struct isdn_ppp_comp_data data; > @@ -589,16 +589,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) > break; > case PPPIOCGCOMPRESSORS: > { > - unsigned long protos[8] = {0,}; > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; s/BITS_PER_LONG/sizeof(long)/duh... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> - unsigned long protos[8] = {0,}; > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; ... > - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) > + if ((r = set_arg(argp, protos, sizeof(protos)))) That change makes a big assumption about the implementation of DECLARE_BITMAP(). Unless it is guaranteed to be implemented as 'unsigned long[]' then you've changed what the code might do. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-10 at 11:42 +0100, David Laight wrote: > > - unsigned long protos[8] = {0,}; > > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; > ... > > - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) > > + if ((r = set_arg(argp, protos, sizeof(protos)))) > > That change makes a big assumption about the implementation > of DECLARE_BITMAP(). > Unless it is guaranteed to be implemented as 'unsigned long[]' > then you've changed what the code might do. Possible, but it's hard to imagine it changing. The = { 0, } should probably be bitmap_zero -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches <joe@perches.com> writes: > @@ -589,16 +589,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) > break; > case PPPIOCGCOMPRESSORS: > { > - unsigned long protos[8] = {0,}; > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; > struct isdn_ppp_compressor *ipc = ipc_head; > + > while (ipc) { > - j = ipc->num / (sizeof(long) * 8); > - i = ipc->num % (sizeof(long) * 8); > - if (j < 8) > - protos[j] |= (0x1 << i); > + if (ipc->num < BITS_PER_LONG * 8) > + set_bit(ipc->num, protos); > ipc = ipc->next; > } > - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) > + if ((r = set_arg(argp, protos, sizeof(protos)))) This changes the bit endianess. Since protos is exported to user space it is an ABI change. Andreas.
On Wed, 2012-10-10 at 14:05 +0200, Andreas Schwab wrote: > Joe Perches <joe@perches.com> writes: > > > @@ -589,16 +589,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) > > break; > > case PPPIOCGCOMPRESSORS: > > { > > - unsigned long protos[8] = {0,}; > > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; > > struct isdn_ppp_compressor *ipc = ipc_head; > > + > > while (ipc) { > > - j = ipc->num / (sizeof(long) * 8); > > - i = ipc->num % (sizeof(long) * 8); > > - if (j < 8) > > - protos[j] |= (0x1 << i); > > + if (ipc->num < BITS_PER_LONG * 8) > > + set_bit(ipc->num, protos); > > ipc = ipc->next; > > } > > - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) > > + if ((r = set_arg(argp, protos, sizeof(protos)))) > > This changes the bit endianess. How does it do that? include/linux/bitops.h:#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) include/linux/bitops.h:#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) include/asm-generic/bitops/atomic.h:static inline void set_bit(int nr, volatile unsigned long *addr) include/asm-generic/bitops/atomic.h-{ include/asm-generic/bitops/atomic.h- unsigned long mask = BIT_MASK(nr); include/asm-generic/bitops/atomic.h- unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); include/asm-generic/bitops/atomic.h- unsigned long flags; include/asm-generic/bitops/atomic.h- include/asm-generic/bitops/atomic.h- _atomic_spin_lock_irqsave(p, flags); include/asm-generic/bitops/atomic.h- *p |= mask; include/asm-generic/bitops/atomic.h- _atomic_spin_unlock_irqrestore(p, flags); include/asm-generic/bitops/atomic.h-} -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry, I was misremembering the history of the bit ops. There has historically been issues with varying bit order, but noadays set_bit is always defined consistently with C shifts. But another issue, set_bit is an atomic operation which may be significantly more expensive than the equivalent C operation. Better use __set_bit when atomicity is not needed. Andreas.
On Wed, 2012-10-10 at 15:58 +0200, Andreas Schwab wrote: > Sorry, I was misremembering the history of the bit ops. There has > historically been issues with varying bit order, but noadays set_bit is > always defined consistently with C shifts. No worries. Anyway, the change was suggested to aid reader comprehension. If it doesn't (and it seems not) then it's not worth it. Anyway, there is still the open question of an overrun/info leak. > > - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) set_arg's 2nd arg is bytes not bits. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Joe Perches > Sent: 10 October 2012 15:42 > To: Andreas Schwab > Cc: Dan Carpenter; Karsten Keil; David S. Miller; Masanari Iida; netdev@vger.kernel.org; kernel- > janitors@vger.kernel.org > Subject: Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl() > > On Wed, 2012-10-10 at 15:58 +0200, Andreas Schwab wrote: > > Sorry, I was misremembering the history of the bit ops. There has > > historically been issues with varying bit order, but noadays set_bit is > > always defined consistently with C shifts. > > No worries. Anyway, the change was suggested to aid > reader comprehension. If it doesn't (and it seems not) > then it's not worth it. > > Anyway, there is still the open question of an overrun/info > leak. > > > > - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) > > set_arg's 2nd arg is bytes not bits. Seems to me the code is expecting 256 bits of data, not any multiple of int, long or anything else. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote: > Seems to me the code is expecting 256 bits of data, not any multiple of int, > long or anything else. include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8]) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote: > > Seems to me the code is expecting 256 bits of data, not any multiple of int, > > long or anything else. > > include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8]) That doesn't mean the whole thing makes any sense on 64bit systems. A whole load of historic code used 'long' to ensure 32bit. Some of that might have crept into Linux sources. Since I suspect there are a maximum of 256 bits on both 32 and 64bit systems, I wouldn't like to guess exactly how any particular 64bit application chooses to check the bitmap. The ioctl constant may be wrong on 64 bit systems. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote: > > On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote: > > > Seems to me the code is expecting 256 bits of data, not any multiple of int, > > > long or anything else. > > > > include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8]) > > That doesn't mean the whole thing makes any sense on 64bit systems. > A whole load of historic code used 'long' to ensure 32bit. > Some of that might have crept into Linux sources. Very true, but it's exported via copy_to_user. > Since I suspect there are a maximum of 256 bits on both 32 and 64bit > systems, I wouldn't like to guess exactly how any particular 64bit > application chooses to check the bitmap. > > The ioctl constant may be wrong on 64 bit systems. shrug. Not much to do about it now. isdn isn't very active. Karsten? What do you think? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Am 10.10.2012 17:52, schrieb Joe Perches: > On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote: >>> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote: >>>> Seems to me the code is expecting 256 bits of data, not any multiple of int, >>>> long or anything else. >>> >>> include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8]) >> >> That doesn't mean the whole thing makes any sense on 64bit systems. >> A whole load of historic code used 'long' to ensure 32bit. >> Some of that might have crept into Linux sources. > > Very true, but it's exported via copy_to_user. > >> Since I suspect there are a maximum of 256 bits on both 32 and 64bit >> systems, I wouldn't like to guess exactly how any particular 64bit >> application chooses to check the bitmap. >> >> The ioctl constant may be wrong on 64 bit systems. > > shrug. Not much to do about it now. > isdn isn't very active. > > Karsten? What do you think? > I use ipppd as testbench to test remote connections via different PPP clients running on a 64 bit system without problems so far - but I did not use any compressions for some years, so maybe this code was never tested on 64 bit and at least not on mixed 32/64 bit systems. If I will find some time, I will check if the compression works. I did not wrote this part, so I cannot say how the code should work correctly out of the box, I need to analyze this first by myself. Karsten -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c index a1e7601..6438d0c 100644 --- a/drivers/isdn/i4l/isdn_ppp.c +++ b/drivers/isdn/i4l/isdn_ppp.c @@ -471,7 +471,7 @@ int isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) { unsigned long val; - int r, i, j; + int r; struct ippp_struct *is; isdn_net_local *lp; struct isdn_ppp_comp_data data; @@ -589,16 +589,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) break; case PPPIOCGCOMPRESSORS: { - unsigned long protos[8] = {0,}; + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; struct isdn_ppp_compressor *ipc = ipc_head; + while (ipc) { - j = ipc->num / (sizeof(long) * 8); - i = ipc->num % (sizeof(long) * 8); - if (j < 8) - protos[j] |= (0x1 << i); + if (ipc->num < BITS_PER_LONG * 8) + set_bit(ipc->num, protos); ipc = ipc->next; } - if ((r = set_arg(argp, protos, 8 * sizeof(long)))) + if ((r = set_arg(argp, protos, sizeof(protos)))) return r; } break;