Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Mon, 26 Jun 2017, Frans Klaver wrote: > On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote: > > > > > > On Sat, 24 Jun 2017, Frans Klaver wrote: > > > > > Hm. For some reason the great mail filtering scheme decided to push > > > this past my inbox :-/ > > > > > > On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches wrote: > > > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > > >> The header field in struct pd_message is declared as an __le16 type. > > > >> The > > > >> data in the message is supposed to be little endian. This means we > > > >> don't > > > >> have to go and shift the individual bytes into position when we're > > > >> filling the buffer, we can just copy the contents right away. As an > > > >> added benefit we don't get fishy results on big endian systems anymore. > > > > > > > > Thanks for pointing this out. > > > > > > > > There are several instances of this class of error. > > > > > > There are other smells around __(le|be) types that show up in staging > > > that might be worth checking in the rest of the kernel as well. e.g. > > > converting to cpu and storing it back into itself (possibly with its > > > bytes reversed), direct assignments without conversion and what else > > > you might have. sparse obviously already flags anything fishy going on > > > with these types, but cannot distinguish between the classes of > > > errors. I'll need to acquaint myself with spatch a bit more to be able > > > to track that down. > > > > If you have concrete code examples, even fake ones, illustrating a class > > of problem, then that would be great. > > Alright, I'll describe two fairly simple cases for starters. > > One class of issue that I have on top of mind is simply > > __le16 val; > > val = le16_to_cpu(val); > > The problem there obviously being that val is supposed to be guaranteed > little endian. Sparse will throw a warning at this. It may also appear > as (or be 'fixed' as) > > __le16 val; > > le16_to_cpus(val); > > Sparse doesn't flag this second version as an issue, while it causes the > same problem. It is especially a potential problem when the value is > stored in driver data. > > Another smell that is prevalent, at least in staging, is > > u16 in; > u16 out; > > out = cpu_to_le16(in); > > or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw > > u64 tmp; > > *(u64*)dst = cpu_to_be64(tmp); > > Now these aren't necessarily problematic. Usually this typo of code is > preparing the data to be sent out in a specific byte ordering, but again > issues may arise if this specifically ordered data is stored somewhere. > > I'll leave it at that for now. OK, thanks! julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote: > > > On Sat, 24 Jun 2017, Frans Klaver wrote: > > > Hm. For some reason the great mail filtering scheme decided to push > > this past my inbox :-/ > > > > On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches wrote: > > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > >> The header field in struct pd_message is declared as an __le16 type. The > > >> data in the message is supposed to be little endian. This means we don't > > >> have to go and shift the individual bytes into position when we're > > >> filling the buffer, we can just copy the contents right away. As an > > >> added benefit we don't get fishy results on big endian systems anymore. > > > > > > Thanks for pointing this out. > > > > > > There are several instances of this class of error. > > > > There are other smells around __(le|be) types that show up in staging > > that might be worth checking in the rest of the kernel as well. e.g. > > converting to cpu and storing it back into itself (possibly with its > > bytes reversed), direct assignments without conversion and what else > > you might have. sparse obviously already flags anything fishy going on > > with these types, but cannot distinguish between the classes of > > errors. I'll need to acquaint myself with spatch a bit more to be able > > to track that down. > > If you have concrete code examples, even fake ones, illustrating a class > of problem, then that would be great. Alright, I'll describe two fairly simple cases for starters. One class of issue that I have on top of mind is simply __le16 val; val = le16_to_cpu(val); The problem there obviously being that val is supposed to be guaranteed little endian. Sparse will throw a warning at this. It may also appear as (or be 'fixed' as) __le16 val; le16_to_cpus(val); Sparse doesn't flag this second version as an issue, while it causes the same problem. It is especially a potential problem when the value is stored in driver data. Another smell that is prevalent, at least in staging, is u16 in; u16 out; out = cpu_to_le16(in); or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw u64 tmp; *(u64*)dst = cpu_to_be64(tmp); Now these aren't necessarily problematic. Usually this typo of code is preparing the data to be sent out in a specific byte ordering, but again issues may arise if this specifically ordered data is stored somewhere. I'll leave it at that for now. Frans ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Mon, 26 Jun 2017, Frans Klaver wrote: > On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall wrote: > > > > > > On Sat, 24 Jun 2017, Frans Klaver wrote: > > > >> Hm. For some reason the great mail filtering scheme decided to push > >> this past my inbox :-/ > >> > >> On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches wrote: > >> > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > >> >> The header field in struct pd_message is declared as an __le16 type. The > >> >> data in the message is supposed to be little endian. This means we don't > >> >> have to go and shift the individual bytes into position when we're > >> >> filling the buffer, we can just copy the contents right away. As an > >> >> added benefit we don't get fishy results on big endian systems anymore. > >> > > >> > Thanks for pointing this out. > >> > > >> > There are several instances of this class of error. > >> > >> There are other smells around __(le|be) types that show up in staging > >> that might be worth checking in the rest of the kernel as well. e.g. > >> converting to cpu and storing it back into itself (possibly with its > >> bytes reversed), direct assignments without conversion and what else > >> you might have. sparse obviously already flags anything fishy going on > >> with these types, but cannot distinguish between the classes of > >> errors. I'll need to acquaint myself with spatch a bit more to be able > >> to track that down. > > > > If you have concrete code examples, even fake ones, illustrating a class > > of problem, then that would be great. > > I'll see if I can produce some somewhere this week. Thanks. julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall wrote: > > > On Sat, 24 Jun 2017, Frans Klaver wrote: > >> Hm. For some reason the great mail filtering scheme decided to push >> this past my inbox :-/ >> >> On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches wrote: >> > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: >> >> The header field in struct pd_message is declared as an __le16 type. The >> >> data in the message is supposed to be little endian. This means we don't >> >> have to go and shift the individual bytes into position when we're >> >> filling the buffer, we can just copy the contents right away. As an >> >> added benefit we don't get fishy results on big endian systems anymore. >> > >> > Thanks for pointing this out. >> > >> > There are several instances of this class of error. >> >> There are other smells around __(le|be) types that show up in staging >> that might be worth checking in the rest of the kernel as well. e.g. >> converting to cpu and storing it back into itself (possibly with its >> bytes reversed), direct assignments without conversion and what else >> you might have. sparse obviously already flags anything fishy going on >> with these types, but cannot distinguish between the classes of >> errors. I'll need to acquaint myself with spatch a bit more to be able >> to track that down. > > If you have concrete code examples, even fake ones, illustrating a class > of problem, then that would be great. I'll see if I can produce some somewhere this week. Thanks, Frans ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Sat, 24 Jun 2017, Frans Klaver wrote: > Hm. For some reason the great mail filtering scheme decided to push > this past my inbox :-/ > > On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches wrote: > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > >> The header field in struct pd_message is declared as an __le16 type. The > >> data in the message is supposed to be little endian. This means we don't > >> have to go and shift the individual bytes into position when we're > >> filling the buffer, we can just copy the contents right away. As an > >> added benefit we don't get fishy results on big endian systems anymore. > > > > Thanks for pointing this out. > > > > There are several instances of this class of error. > > There are other smells around __(le|be) types that show up in staging > that might be worth checking in the rest of the kernel as well. e.g. > converting to cpu and storing it back into itself (possibly with its > bytes reversed), direct assignments without conversion and what else > you might have. sparse obviously already flags anything fishy going on > with these types, but cannot distinguish between the classes of > errors. I'll need to acquaint myself with spatch a bit more to be able > to track that down. If you have concrete code examples, even fake ones, illustrating a class of problem, then that would be great. thanks, julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
Hm. For some reason the great mail filtering scheme decided to push this past my inbox :-/ On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches wrote: > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: >> The header field in struct pd_message is declared as an __le16 type. The >> data in the message is supposed to be little endian. This means we don't >> have to go and shift the individual bytes into position when we're >> filling the buffer, we can just copy the contents right away. As an >> added benefit we don't get fishy results on big endian systems anymore. > > Thanks for pointing this out. > > There are several instances of this class of error. There are other smells around __(le|be) types that show up in staging that might be worth checking in the rest of the kernel as well. e.g. converting to cpu and storing it back into itself (possibly with its bytes reversed), direct assignments without conversion and what else you might have. sparse obviously already flags anything fishy going on with these types, but cannot distinguish between the classes of errors. I'll need to acquaint myself with spatch a bit more to be able to track that down. Thanks, Frans ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Sat, 2017-06-17 at 08:00 +0200, Julia Lawall wrote: > On Fri, 16 Jun 2017, Joe Perches wrote: > > On Sat, 2017-06-17 at 07:23 +0200, Julia Lawall wrote: > > > On Fri, 16 Jun 2017, Joe Perches wrote: > > > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > > > > The header field in struct pd_message is declared as an __le16 type. > > > > > The > > > > > data in the message is supposed to be little endian. This means we > > > > > don't > > > > > have to go and shift the individual bytes into position when we're > > > > > filling the buffer, we can just copy the contents right away. As an > > > > > added benefit we don't get fishy results on big endian systems > > > > > anymore. > > > > > > > > Thanks for pointing this out. > > > > > > > > There are several instances of this class of error. > > > > > > > > Here's a cocci script to find them. > > > > > > > > This is best used with cocci's --all-includes option like: > > > > > > > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . > > > > [ many defects...] > > > > Probably would have been better as [ many possible defects... ] > > OK > > > > > $ cat lebe_bitshifts.cocci > > > > @@ > > > > typedef __le16, __le32, __le64, __be16, __be32, __be64; > > > > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > > > > expression b; > > > > @@ > > > > > > > > * a << b > > > > [etc...] > > > > > Is this always a problem? > > > > No, not always. > > > > If the CPU is the equivalent endian, the bitshift is fine. > > It can't be known if the code is only compiled on a > > single cpu type. It is rather odd though to use endian > > notation if the code is compiled for a single cpu type. > > Is there some way to know from the code if it is compiled for a single cou > type? No easy way as far as I can tell. I believe it'd require parsing Kconfig. > > > Would it be useful to add this to the scripts > > > in the kernel? > > > > Maybe. > > If there are a lot of false positives, it could be a nuisance... I believe the most likely false positives would be in arch/ code > > btw: is there a way for the operators to be surrounded by > > some \( \| \) or some other bracket style so it could > > be written with a single test? > > > > Something like: > > > > @@ > > typedef __le16, __le32, __le64, __be16, __be32, __be64; > > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > > expression b; > > @@ > > > > * a [<<|<<=|>>|>>=] b > > Partly. You can define > > binary operator bop = {<<,>>}; thanks. btw: After a couple hours with this script, I got a segmentation fault Here's the output I got running $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . diff -u -p ./net/dsa/tag_qca.c /tmp/nothing/net/dsa/tag_qca.c --- ./net/dsa/tag_qca.c +++ /tmp/nothing/net/dsa/tag_qca.c @@ -84,7 +84,6 @@ static struct sk_buff *qca_tag_rcv(struc hdr = ntohs(*phdr); /* Make sure the version is correct */ - ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S; if (unlikely(ver != QCA_HDR_VERSION)) return NULL; diff -u -p ./arch/mips/pci/pci-lantiq.c /tmp/nothing/arch/mips/pci/pci-lantiq.c --- ./arch/mips/pci/pci-lantiq.c +++ /tmp/nothing/arch/mips/pci/pci-lantiq.c @@ -151,7 +151,6 @@ static int ltq_pci_startup(struct platfo /* setup the request mask */ req_mask = of_get_property(node, "req-mask", NULL); if (req_mask) - temp_buffer &= ~((*req_mask & 0xf) << 16); else temp_buffer &= ~0xf; /* enable internal arbiter */ diff -u -p ./arch/powerpc/platforms/powernv/opal-lpc.c /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c --- ./arch/powerpc/platforms/powernv/opal-lpc.c +++ /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c @@ -44,7 +44,6 @@ static __le16 __opal_lpc_inw(unsigned lo if (opal_lpc_chip_id < 0 || port > 0xfffe) return 0x; if (port & 1) - return (__le16)opal_lpc_inb(port) << 8 | opal_lpc_inb(port + 1); rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, &data, 2); return rc ? 0x : be32_to_cpu(data); } @@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo if (opal_lpc_chip_id < 0 || port > 0xfffc) return 0x; if (port & 3) - return (__le32)opal_lpc_inb(port) << 24 | - (__le32)opal_lpc_inb(port + 1) << 16 | - (__le32)opal_lpc_inb(port + 2) << 8 | opal_lpc_inb(port + 3); rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, &data, 4); return rc ? 0x : be32_to_cpu(data); @@ -86,7 +82,6 @@ static void __opal_lpc_outw(__le16 val, if (opal_lpc_chip_id < 0 || port > 0xfffe) return; if (port & 1) { - opal_lpc_outb(val >> 8, port); opal_lpc_outb(val , port + 1);
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Fri, 16 Jun 2017, Joe Perches wrote: > On Sat, 2017-06-17 at 08:00 +0200, Julia Lawall wrote: > > On Fri, 16 Jun 2017, Joe Perches wrote: > > > On Sat, 2017-06-17 at 07:23 +0200, Julia Lawall wrote: > > > > On Fri, 16 Jun 2017, Joe Perches wrote: > > > > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > > > > > The header field in struct pd_message is declared as an __le16 > > > > > > type. The > > > > > > data in the message is supposed to be little endian. This means we > > > > > > don't > > > > > > have to go and shift the individual bytes into position when we're > > > > > > filling the buffer, we can just copy the contents right away. As an > > > > > > added benefit we don't get fishy results on big endian systems > > > > > > anymore. > > > > > > > > > > Thanks for pointing this out. > > > > > > > > > > There are several instances of this class of error. > > > > > > > > > > Here's a cocci script to find them. > > > > > > > > > > This is best used with cocci's --all-includes option like: > > > > > > > > > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . > > > > > [ many defects...] > > > > > > Probably would have been better as [ many possible defects... ] > > > > OK > > > > > > > $ cat lebe_bitshifts.cocci > > > > > @@ > > > > > typedef __le16, __le32, __le64, __be16, __be32, __be64; > > > > > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > > > > > expression b; > > > > > @@ > > > > > > > > > > * a << b > > > > > > [etc...] > > > > > > > Is this always a problem? > > > > > > No, not always. > > > > > > If the CPU is the equivalent endian, the bitshift is fine. > > > It can't be known if the code is only compiled on a > > > single cpu type. It is rather odd though to use endian > > > notation if the code is compiled for a single cpu type. > > > > Is there some way to know from the code if it is compiled for a single cou > > type? > > No easy way as far as I can tell. > I believe it'd require parsing Kconfig. > > > > > Would it be useful to add this to the scripts > > > > in the kernel? > > > > > > Maybe. > > > > If there are a lot of false positives, it could be a nuisance... > > I believe the most likely false positives would be in arch/ code > > > > btw: is there a way for the operators to be surrounded by > > > some \( \| \) or some other bracket style so it could > > > be written with a single test? > > > > > > Something like: > > > > > > @@ > > > typedef __le16, __le32, __le64, __be16, __be32, __be64; > > > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > > > expression b; > > > @@ > > > > > > * a [<<|<<=|>>|>>=] b > > > > Partly. You can define > > > > binary operator bop = {<<,>>}; > > thanks. > > btw: After a couple hours with this script, I got a segmentation fault > > Here's the output I got running > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . Weird. I will try it. thanks, julia > diff -u -p ./net/dsa/tag_qca.c /tmp/nothing/net/dsa/tag_qca.c > --- ./net/dsa/tag_qca.c > +++ /tmp/nothing/net/dsa/tag_qca.c > @@ -84,7 +84,6 @@ static struct sk_buff *qca_tag_rcv(struc > hdr = ntohs(*phdr); > > /* Make sure the version is correct */ > - ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S; > if (unlikely(ver != QCA_HDR_VERSION)) > return NULL; > > diff -u -p ./arch/mips/pci/pci-lantiq.c > /tmp/nothing/arch/mips/pci/pci-lantiq.c > --- ./arch/mips/pci/pci-lantiq.c > +++ /tmp/nothing/arch/mips/pci/pci-lantiq.c > @@ -151,7 +151,6 @@ static int ltq_pci_startup(struct platfo > /* setup the request mask */ > req_mask = of_get_property(node, "req-mask", NULL); > if (req_mask) > - temp_buffer &= ~((*req_mask & 0xf) << 16); > else > temp_buffer &= ~0xf; > /* enable internal arbiter */ > diff -u -p ./arch/powerpc/platforms/powernv/opal-lpc.c > /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c > --- ./arch/powerpc/platforms/powernv/opal-lpc.c > +++ /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c > @@ -44,7 +44,6 @@ static __le16 __opal_lpc_inw(unsigned lo > if (opal_lpc_chip_id < 0 || port > 0xfffe) > return 0x; > if (port & 1) > - return (__le16)opal_lpc_inb(port) << 8 | opal_lpc_inb(port + 1); > rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, &data, 2); > return rc ? 0x : be32_to_cpu(data); > } > @@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo > if (opal_lpc_chip_id < 0 || port > 0xfffc) > return 0x; > if (port & 3) > - return (__le32)opal_lpc_inb(port) << 24 | > -(__le32)opal_lpc_inb(port + 1) << 16 | > -(__le32)opal_lpc_inb(port + 2) << 8 | > opal_lpc_inb(port + 3); > rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, &data, 4); > return rc ? 0x : be32_to_cpu(data);
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Fri, 16 Jun 2017, Joe Perches wrote: > On Sat, 2017-06-17 at 07:23 +0200, Julia Lawall wrote: > > On Fri, 16 Jun 2017, Joe Perches wrote: > > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > > > The header field in struct pd_message is declared as an __le16 type. The > > > > data in the message is supposed to be little endian. This means we don't > > > > have to go and shift the individual bytes into position when we're > > > > filling the buffer, we can just copy the contents right away. As an > > > > added benefit we don't get fishy results on big endian systems anymore. > > > > > > Thanks for pointing this out. > > > > > > There are several instances of this class of error. > > > > > > Here's a cocci script to find them. > > > > > > This is best used with cocci's --all-includes option like: > > > > > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . > > > [ many defects...] > > Probably would have been better as [ many possible defects... ] OK > > > $ cat lebe_bitshifts.cocci > > > @@ > > > typedef __le16, __le32, __le64, __be16, __be32, __be64; > > > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > > > expression b; > > > @@ > > > > > > * a << b > > [etc...] > > > Is this always a problem? > > No, not always. > > If the CPU is the equivalent endian, the bitshift is fine. > It can't be known if the code is only compiled on a > single cpu type. It is rather odd though to use endian > notation if the code is compiled for a single cpu type. Is there some way to know from the code if it is compiled for a single cou type? > > Would it be useful to add this to the scripts > > in the kernel? > > Maybe. If there are a lot of false positives, it could be a nuisance... > btw: is there a way for the operators to be surrounded by > some \( \| \) or some other bracket style so it could > be written with a single test? > > Something like: > > @@ > typedef __le16, __le32, __le64, __be16, __be32, __be64; > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > expression b; > @@ > > * a [<<|<<=|>>|>>=] b Partly. You can define binary operator bop = {<<,>>}; or assignment operator aop = {<<=,>>=}; to make two rules instead of four. julia___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Sat, 2017-06-17 at 07:23 +0200, Julia Lawall wrote: > On Fri, 16 Jun 2017, Joe Perches wrote: > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > > The header field in struct pd_message is declared as an __le16 type. The > > > data in the message is supposed to be little endian. This means we don't > > > have to go and shift the individual bytes into position when we're > > > filling the buffer, we can just copy the contents right away. As an > > > added benefit we don't get fishy results on big endian systems anymore. > > > > Thanks for pointing this out. > > > > There are several instances of this class of error. > > > > Here's a cocci script to find them. > > > > This is best used with cocci's --all-includes option like: > > > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . > > [ many defects...] Probably would have been better as [ many possible defects... ] > > $ cat lebe_bitshifts.cocci > > @@ > > typedef __le16, __le32, __le64, __be16, __be32, __be64; > > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > > expression b; > > @@ > > > > * a << b [etc...] > Is this always a problem? No, not always. If the CPU is the equivalent endian, the bitshift is fine. It can't be known if the code is only compiled on a single cpu type. It is rather odd though to use endian notation if the code is compiled for a single cpu type. > Would it be useful to add this to the scripts > in the kernel? Maybe. btw: is there a way for the operators to be surrounded by some \( \| \) or some other bracket style so it could be written with a single test? Something like: @@ typedef __le16, __le32, __le64, __be16, __be32, __be64; { __le16, __le32, __le64, __be16, __be32, __be64 } a; expression b; @@ * a [<<|<<=|>>|>>=] b ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Fri, 16 Jun 2017, Joe Perches wrote: > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > > The header field in struct pd_message is declared as an __le16 type. The > > data in the message is supposed to be little endian. This means we don't > > have to go and shift the individual bytes into position when we're > > filling the buffer, we can just copy the contents right away. As an > > added benefit we don't get fishy results on big endian systems anymore. > > Thanks for pointing this out. > > There are several instances of this class of error. > > Here's a cocci script to find them. > > This is best used with cocci's --all-includes option like: > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . > [ many defects...] > > $ cat lebe_bitshifts.cocci > @@ > typedef __le16, __le32, __le64, __be16, __be32, __be64; > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > expression b; > @@ > > * a << b > > @@ > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > expression b; > @@ > > * a <<= b > > @@ > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > expression b; > @@ > > * a >> b > > @@ > { __le16, __le32, __le64, __be16, __be32, __be64 } a; > expression b; > @@ > > * a >>= b Is this always a problem? Would it be useful to add this to the scripts in the kernel? julia___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]
On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote: > The header field in struct pd_message is declared as an __le16 type. The > data in the message is supposed to be little endian. This means we don't > have to go and shift the individual bytes into position when we're > filling the buffer, we can just copy the contents right away. As an > added benefit we don't get fishy results on big endian systems anymore. Thanks for pointing this out. There are several instances of this class of error. Here's a cocci script to find them. This is best used with cocci's --all-includes option like: $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci . [ many defects...] $ cat lebe_bitshifts.cocci @@ typedef __le16, __le32, __le64, __be16, __be32, __be64; { __le16, __le32, __le64, __be16, __be32, __be64 } a; expression b; @@ * a << b @@ { __le16, __le32, __le64, __be16, __be32, __be64 } a; expression b; @@ * a <<= b @@ { __le16, __le32, __le64, __be16, __be32, __be64 } a; expression b; @@ * a >> b @@ { __le16, __le32, __le64, __be16, __be32, __be64 } a; expression b; @@ * a >>= b $ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel