endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Joe Perches]

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
$

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]



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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Joe Perches]

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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]



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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Joe Perches]

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 &= ~0xf0000;
 	/* 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 0xffff;
 	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 ? 0xffff : be32_to_cpu(data);
 }
@@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo
 	if (opal_lpc_chip_id < 0 || port > 0xfffc)
 		return 0xffffffff;
 	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 ? 0xffffffff : 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);
 		return;
 	}
@@ -103,9 +98,6 @@ static void __opal_lpc_outl(__le32 val,
 	if (opal_lpc_chip_id < 0 || port > 0xfffc)
 		return;
 	if (port & 3) {
-		opal_lpc_outb(val >> 24, port);
-		opal_lpc_outb(val >> 16, port + 1);
-		opal_lpc_outb(val >>  8, port + 2);
 		opal_lpc_outb(val      , port + 3);
 		return;
 	}
diff -u -p ./drivers/net/geneve.c /tmp/nothing/drivers/net/geneve.c
--- ./drivers/net/geneve.c
+++ /tmp/nothing/drivers/net/geneve.c
@@ -93,8 +93,6 @@ static __be64 vni_to_tunnel_id(const __u
 static void tunnel_id_to_vni(__be64 tun_id, __u8 *vni)
 {
 #ifdef __BIG_ENDIAN
-	vni[0] = (__force __u8)(tun_id >> 16);
-	vni[1] = (__force __u8)(tun_id >> 8);
 	vni[2] = (__force __u8)tun_id;
 #else
 	vni[0] = (__force __u8)((__force u64)tun_id >> 40);
diff -u -p ./drivers/net/ethernet/atheros/atl1c/atl1c_main.c /tmp/nothing/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
--- ./drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ /tmp/nothing/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1785,7 +1785,6 @@ static void atl1c_clean_rfd(struct atl1c
 	u16 rfd_index;
 	struct atl1c_buffer *buffer_info = rfd_ring->buffer_info;
 
-	rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
 			RRS_RX_RFD_INDEX_MASK;
 	for (i = 0; i < num; i++) {
 		buffer_info[rfd_index].skb = NULL;
@@ -1816,7 +1815,6 @@ static void atl1c_clean_rx_irq(struct at
 			break;
 		rrs = ATL1C_RRD_DESC(rrd_ring, rrd_ring->next_to_clean);
 		if (likely(RRS_RXD_IS_VALID(rrs->word3))) {
-			rfd_num = (rrs->word0 >> RRS_RX_RFD_CNT_SHIFT) &
 				RRS_RX_RFD_CNT_MASK;
 			if (unlikely(rfd_num != 1))
 				/* TODO support mul rfd*/
@@ -1838,11 +1836,9 @@ rrs_checked:
 			continue;
 		}
 
-		length = le16_to_cpu((rrs->word3 >> RRS_PKT_SIZE_SHIFT) &
 				RRS_PKT_SIZE_MASK);
 		/* Good Receive */
 		if (likely(rfd_num = 1)) {
-			rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
 					RRS_RX_RFD_INDEX_MASK;
 			buffer_info = &rfd_ring->buffer_info[rfd_index];
 			pci_unmap_single(pdev, buffer_info->dma,
diff -u -p ./drivers/net/ethernet/atheros/atl1e/atl1e_main.c /tmp/nothing/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
--- ./drivers/net/ethernet/atheros/atl1e/atl1e_main.c
+++ /tmp/nothing/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
@@ -1449,7 +1449,6 @@ static void atl1e_clean_rx_irq(struct at
 				}
 			}
 
-			packet_size = ((prrs->word1 >> RRS_PKT_SIZE_SHIFT) &
 					RRS_PKT_SIZE_MASK);
 			if (likely(!(netdev->features & NETIF_F_RXFCS)))
 				packet_size -= 4; /* CRC */
@@ -1477,7 +1476,6 @@ static void atl1e_clean_rx_irq(struct at
 skip_pkt:
 	/* skip current packet whether it's ok or not. */
 			rx_page->read_offset +-				(((u32)((prrs->word1 >> RRS_PKT_SIZE_SHIFT) &
 				RRS_PKT_SIZE_MASK) +
 				sizeof(struct atl1e_recv_ret_status) + 31) &
 						0xFFFFFFE0);
@@ -1716,7 +1714,6 @@ static int atl1e_tx_map(struct atl1e_ada
 	int ring_end;
 
 	nr_frags = skb_shinfo(skb)->nr_frags;
-	segment = (tpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK;
 	if (segment) {
 		/* TSO */
 		map_len = hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
@@ -1831,7 +1828,6 @@ static int atl1e_tx_map(struct atl1e_ada
 		}
 	}
 
-	if ((tpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK)
 		/* note this one is a tcp header */
 		tpd->word3 |= 1 << TPD_HDRFLAG_SHIFT;
 	/* The last tpd */
diff -u -p ./drivers/net/ethernet/atheros/atlx/atl1.c /tmp/nothing/drivers/net/ethernet/atheros/atlx/atl1.c
--- ./drivers/net/ethernet/atheros/atlx/atl1.c
+++ /tmp/nothing/drivers/net/ethernet/atheros/atlx/atl1.c
@@ -2224,7 +2224,6 @@ static void atl1_tx_map(struct atl1_adap
 	/* put skb in last TPD */
 	buffer_info->skb = NULL;
 
-	retval = (ptpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK;
 	if (retval) {
 		/* TSO */
 		hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
@@ -2328,7 +2327,6 @@ static void atl1_tx_queue(struct atl1_ad
 		 * if this is the first packet in a TSO chain, set
 		 * TPD_HDRFLAG, otherwise, clear it.
 		 */
-		val = (tpd->word3 >> TPD_SEGMENT_EN_SHIFT) &
 			TPD_SEGMENT_EN_MASK;
 		if (val) {
 			if (!j)
diff -u -p ./drivers/net/ethernet/3com/3c509.c /tmp/nothing/drivers/net/ethernet/3com/3c509.c
--- ./drivers/net/ethernet/3com/3c509.c
+++ /tmp/nothing/drivers/net/ethernet/3com/3c509.c
@@ -255,9 +255,6 @@ static int el3_isa_id_sequence(__be16 *p
 			    ether_addr_equal((u8 *)phys_addr, el3_devs[i]->dev_addr)) {
 				if (el3_debug > 3)
 					pr_debug("3c509 with address %02x %02x %02x %02x %02x %02x was found by ISAPnP\n",
-						phys_addr[0] & 0xff, phys_addr[0] >> 8,
-						phys_addr[1] & 0xff, phys_addr[1] >> 8,
-						phys_addr[2] & 0xff, phys_addr[2] >> 8);
 				/* Set the adaptor tag so that the next card can be found. */
 				outb(0xd0 + ++current_tag, id_port);
 				return 2;
diff -u -p ./drivers/net/ethernet/qualcomm/qca_7k_common.c /tmp/nothing/drivers/net/ethernet/qualcomm/qca_7k_common.c
--- ./drivers/net/ethernet/qualcomm/qca_7k_common.c
+++ /tmp/nothing/drivers/net/ethernet/qualcomm/qca_7k_common.c
@@ -42,7 +42,6 @@ qcafrm_create_header(u8 *buf, u16 length
 	buf[2] = 0xAA;
 	buf[3] = 0xAA;
 	buf[4] = len & 0xff;
-	buf[5] = (len >> 8) & 0xff;
 	buf[6] = 0;
 	buf[7] = 0;
 
diff -u -p ./drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c /tmp/nothing/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
--- ./drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
+++ /tmp/nothing/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
@@ -862,7 +862,6 @@ nx_get_fw_version(struct netxen_adapter
 		if (ret != 3)
 			return 0;
 
-		return major + (minor << 8) + (sub << 16);
 
 	} else
 		return cpu_to_le32(*(u32 *)&fw->data[NX_FW_VERSION_OFFSET]);
@@ -877,8 +876,6 @@ nx_get_bios_version(struct netxen_adapte
 	if (adapter->fw_type = NX_UNIFIED_ROMIMAGE) {
 		bios_ver = cpu_to_le32(*((u32 *) (&fw->data[prd_off])
 						+ NX_UNI_BIOS_VERSION_OFF));
-		return (bios_ver << 16) + ((bios_ver >> 8) & 0xff00) +
-							(bios_ver >> 24);
 	} else
 		return cpu_to_le32(*(u32 *)&fw->data[NX_BIOS_VERSION_OFFSET]);
 
diff -u -p ./drivers/net/ethernet/intel/e100.c /tmp/nothing/drivers/net/ethernet/intel/e100.c
--- ./drivers/net/ethernet/intel/e100.c
+++ /tmp/nothing/drivers/net/ethernet/intel/e100.c
@@ -1423,7 +1423,6 @@ static int e100_phy_check_without_mii(st
 	u8 phy_type;
 	int without_mii;
 
-	phy_type = (nic->eeprom[eeprom_phy_iface] >> 8) & 0x0f;
 
 	switch (phy_type) {
 	case NoSuchPhy: /* Non-MII PHY; UNTESTED! */
diff -u -p ./drivers/crypto/sunxi-ss/sun4i-ss-hash.c /tmp/nothing/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
--- ./drivers/crypto/sunxi-ss/sun4i-ss-hash.c
+++ /tmp/nothing/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
@@ -434,7 +434,6 @@ hash_final:
 	if (op->mode = SS_OP_SHA1) {
 		bits = cpu_to_be64(op->byte_count << 3);
 		bf[j++] = bits & 0xffffffff;
-		bf[j++] = (bits >> 32) & 0xffffffff;
 	} else {
 		bf[j++] = (op->byte_count << 3) & 0xffffffff;
 		bf[j++] = (op->byte_count >> 29) & 0xffffffff;
diff -u -p ./drivers/staging/rtl8723bs/core/rtw_wlan_util.c /tmp/nothing/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
--- ./drivers/staging/rtl8723bs/core/rtw_wlan_util.c
+++ /tmp/nothing/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
@@ -2258,9 +2258,6 @@ void rtw_get_current_ip_address(struct a
 			struct in_ifaddr *my_ifa_list = my_ip_ptr->ifa_list;
 			if (my_ifa_list != NULL) {
 				ipaddress[0] = my_ifa_list->ifa_address & 0xFF;
-				ipaddress[1] = (my_ifa_list->ifa_address >> 8) & 0xFF;
-				ipaddress[2] = (my_ifa_list->ifa_address >> 16) & 0xFF;
-				ipaddress[3] = my_ifa_list->ifa_address >> 24;
 				DBG_871X("%s: %d.%d.%d.%d =====\n", __func__,
 						ipaddress[0], ipaddress[1], ipaddress[2], ipaddress[3]);
 				memcpy(pcurrentip, ipaddress, 4);
diff -u -p ./drivers/staging/typec/fusb302/fusb302.c /tmp/nothing/drivers/staging/typec/fusb302/fusb302.c
--- ./drivers/staging/typec/fusb302/fusb302.c
+++ /tmp/nothing/drivers/staging/typec/fusb302/fusb302.c
@@ -1040,7 +1040,6 @@ static int fusb302_pd_send_message(struc
 	/* packsym tells the FUSB302 chip that the next X bytes are payload */
 	buf[pos++] = FUSB302_TKN_PACKSYM | (len & 0x1F);
 	buf[pos++] = msg->header & 0xFF;
-	buf[pos++] = (msg->header >> 8) & 0xFF;
 
 	len -= 2;
 	memcpy(&buf[pos], msg->payload, len);
Segmentation fault (core dumped)

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 14175 bytes --]



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 &= ~0xf0000;
>  	/* 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 0xffff;
>  	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 ? 0xffff : be32_to_cpu(data);
>  }
> @@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo
>  	if (opal_lpc_chip_id < 0 || port > 0xfffc)
>  		return 0xffffffff;
>  	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 ? 0xffffffff : 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);
>  		return;
>  	}
> @@ -103,9 +98,6 @@ static void __opal_lpc_outl(__le32 val,
>  	if (opal_lpc_chip_id < 0 || port > 0xfffc)
>  		return;
>  	if (port & 3) {
> -		opal_lpc_outb(val >> 24, port);
> -		opal_lpc_outb(val >> 16, port + 1);
> -		opal_lpc_outb(val >>  8, port + 2);
>  		opal_lpc_outb(val      , port + 3);
>  		return;
>  	}
> diff -u -p ./drivers/net/geneve.c /tmp/nothing/drivers/net/geneve.c
> --- ./drivers/net/geneve.c
> +++ /tmp/nothing/drivers/net/geneve.c
> @@ -93,8 +93,6 @@ static __be64 vni_to_tunnel_id(const __u
>  static void tunnel_id_to_vni(__be64 tun_id, __u8 *vni)
>  {
>  #ifdef __BIG_ENDIAN
> -	vni[0] = (__force __u8)(tun_id >> 16);
> -	vni[1] = (__force __u8)(tun_id >> 8);
>  	vni[2] = (__force __u8)tun_id;
>  #else
>  	vni[0] = (__force __u8)((__force u64)tun_id >> 40);
> diff -u -p ./drivers/net/ethernet/atheros/atl1c/atl1c_main.c /tmp/nothing/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> --- ./drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ /tmp/nothing/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1785,7 +1785,6 @@ static void atl1c_clean_rfd(struct atl1c
>  	u16 rfd_index;
>  	struct atl1c_buffer *buffer_info = rfd_ring->buffer_info;
>
> -	rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
>  			RRS_RX_RFD_INDEX_MASK;
>  	for (i = 0; i < num; i++) {
>  		buffer_info[rfd_index].skb = NULL;
> @@ -1816,7 +1815,6 @@ static void atl1c_clean_rx_irq(struct at
>  			break;
>  		rrs = ATL1C_RRD_DESC(rrd_ring, rrd_ring->next_to_clean);
>  		if (likely(RRS_RXD_IS_VALID(rrs->word3))) {
> -			rfd_num = (rrs->word0 >> RRS_RX_RFD_CNT_SHIFT) &
>  				RRS_RX_RFD_CNT_MASK;
>  			if (unlikely(rfd_num != 1))
>  				/* TODO support mul rfd*/
> @@ -1838,11 +1836,9 @@ rrs_checked:
>  			continue;
>  		}
>
> -		length = le16_to_cpu((rrs->word3 >> RRS_PKT_SIZE_SHIFT) &
>  				RRS_PKT_SIZE_MASK);
>  		/* Good Receive */
>  		if (likely(rfd_num == 1)) {
> -			rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
>  					RRS_RX_RFD_INDEX_MASK;
>  			buffer_info = &rfd_ring->buffer_info[rfd_index];
>  			pci_unmap_single(pdev, buffer_info->dma,
> diff -u -p ./drivers/net/ethernet/atheros/atl1e/atl1e_main.c /tmp/nothing/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
> --- ./drivers/net/ethernet/atheros/atl1e/atl1e_main.c
> +++ /tmp/nothing/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
> @@ -1449,7 +1449,6 @@ static void atl1e_clean_rx_irq(struct at
>  				}
>  			}
>
> -			packet_size = ((prrs->word1 >> RRS_PKT_SIZE_SHIFT) &
>  					RRS_PKT_SIZE_MASK);
>  			if (likely(!(netdev->features & NETIF_F_RXFCS)))
>  				packet_size -= 4; /* CRC */
> @@ -1477,7 +1476,6 @@ static void atl1e_clean_rx_irq(struct at
>  skip_pkt:
>  	/* skip current packet whether it's ok or not. */
>  			rx_page->read_offset +=
> -				(((u32)((prrs->word1 >> RRS_PKT_SIZE_SHIFT) &
>  				RRS_PKT_SIZE_MASK) +
>  				sizeof(struct atl1e_recv_ret_status) + 31) &
>  						0xFFFFFFE0);
> @@ -1716,7 +1714,6 @@ static int atl1e_tx_map(struct atl1e_ada
>  	int ring_end;
>
>  	nr_frags = skb_shinfo(skb)->nr_frags;
> -	segment = (tpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK;
>  	if (segment) {
>  		/* TSO */
>  		map_len = hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
> @@ -1831,7 +1828,6 @@ static int atl1e_tx_map(struct atl1e_ada
>  		}
>  	}
>
> -	if ((tpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK)
>  		/* note this one is a tcp header */
>  		tpd->word3 |= 1 << TPD_HDRFLAG_SHIFT;
>  	/* The last tpd */
> diff -u -p ./drivers/net/ethernet/atheros/atlx/atl1.c /tmp/nothing/drivers/net/ethernet/atheros/atlx/atl1.c
> --- ./drivers/net/ethernet/atheros/atlx/atl1.c
> +++ /tmp/nothing/drivers/net/ethernet/atheros/atlx/atl1.c
> @@ -2224,7 +2224,6 @@ static void atl1_tx_map(struct atl1_adap
>  	/* put skb in last TPD */
>  	buffer_info->skb = NULL;
>
> -	retval = (ptpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK;
>  	if (retval) {
>  		/* TSO */
>  		hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
> @@ -2328,7 +2327,6 @@ static void atl1_tx_queue(struct atl1_ad
>  		 * if this is the first packet in a TSO chain, set
>  		 * TPD_HDRFLAG, otherwise, clear it.
>  		 */
> -		val = (tpd->word3 >> TPD_SEGMENT_EN_SHIFT) &
>  			TPD_SEGMENT_EN_MASK;
>  		if (val) {
>  			if (!j)
> diff -u -p ./drivers/net/ethernet/3com/3c509.c /tmp/nothing/drivers/net/ethernet/3com/3c509.c
> --- ./drivers/net/ethernet/3com/3c509.c
> +++ /tmp/nothing/drivers/net/ethernet/3com/3c509.c
> @@ -255,9 +255,6 @@ static int el3_isa_id_sequence(__be16 *p
>  			    ether_addr_equal((u8 *)phys_addr, el3_devs[i]->dev_addr)) {
>  				if (el3_debug > 3)
>  					pr_debug("3c509 with address %02x %02x %02x %02x %02x %02x was found by ISAPnP\n",
> -						phys_addr[0] & 0xff, phys_addr[0] >> 8,
> -						phys_addr[1] & 0xff, phys_addr[1] >> 8,
> -						phys_addr[2] & 0xff, phys_addr[2] >> 8);
>  				/* Set the adaptor tag so that the next card can be found. */
>  				outb(0xd0 + ++current_tag, id_port);
>  				return 2;
> diff -u -p ./drivers/net/ethernet/qualcomm/qca_7k_common.c /tmp/nothing/drivers/net/ethernet/qualcomm/qca_7k_common.c
> --- ./drivers/net/ethernet/qualcomm/qca_7k_common.c
> +++ /tmp/nothing/drivers/net/ethernet/qualcomm/qca_7k_common.c
> @@ -42,7 +42,6 @@ qcafrm_create_header(u8 *buf, u16 length
>  	buf[2] = 0xAA;
>  	buf[3] = 0xAA;
>  	buf[4] = len & 0xff;
> -	buf[5] = (len >> 8) & 0xff;
>  	buf[6] = 0;
>  	buf[7] = 0;
>
> diff -u -p ./drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c /tmp/nothing/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> --- ./drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> +++ /tmp/nothing/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> @@ -862,7 +862,6 @@ nx_get_fw_version(struct netxen_adapter
>  		if (ret != 3)
>  			return 0;
>
> -		return major + (minor << 8) + (sub << 16);
>
>  	} else
>  		return cpu_to_le32(*(u32 *)&fw->data[NX_FW_VERSION_OFFSET]);
> @@ -877,8 +876,6 @@ nx_get_bios_version(struct netxen_adapte
>  	if (adapter->fw_type == NX_UNIFIED_ROMIMAGE) {
>  		bios_ver = cpu_to_le32(*((u32 *) (&fw->data[prd_off])
>  						+ NX_UNI_BIOS_VERSION_OFF));
> -		return (bios_ver << 16) + ((bios_ver >> 8) & 0xff00) +
> -							(bios_ver >> 24);
>  	} else
>  		return cpu_to_le32(*(u32 *)&fw->data[NX_BIOS_VERSION_OFFSET]);
>
> diff -u -p ./drivers/net/ethernet/intel/e100.c /tmp/nothing/drivers/net/ethernet/intel/e100.c
> --- ./drivers/net/ethernet/intel/e100.c
> +++ /tmp/nothing/drivers/net/ethernet/intel/e100.c
> @@ -1423,7 +1423,6 @@ static int e100_phy_check_without_mii(st
>  	u8 phy_type;
>  	int without_mii;
>
> -	phy_type = (nic->eeprom[eeprom_phy_iface] >> 8) & 0x0f;
>
>  	switch (phy_type) {
>  	case NoSuchPhy: /* Non-MII PHY; UNTESTED! */
> diff -u -p ./drivers/crypto/sunxi-ss/sun4i-ss-hash.c /tmp/nothing/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
> --- ./drivers/crypto/sunxi-ss/sun4i-ss-hash.c
> +++ /tmp/nothing/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
> @@ -434,7 +434,6 @@ hash_final:
>  	if (op->mode == SS_OP_SHA1) {
>  		bits = cpu_to_be64(op->byte_count << 3);
>  		bf[j++] = bits & 0xffffffff;
> -		bf[j++] = (bits >> 32) & 0xffffffff;
>  	} else {
>  		bf[j++] = (op->byte_count << 3) & 0xffffffff;
>  		bf[j++] = (op->byte_count >> 29) & 0xffffffff;
> diff -u -p ./drivers/staging/rtl8723bs/core/rtw_wlan_util.c /tmp/nothing/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> --- ./drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> +++ /tmp/nothing/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> @@ -2258,9 +2258,6 @@ void rtw_get_current_ip_address(struct a
>  			struct in_ifaddr *my_ifa_list = my_ip_ptr->ifa_list;
>  			if (my_ifa_list != NULL) {
>  				ipaddress[0] = my_ifa_list->ifa_address & 0xFF;
> -				ipaddress[1] = (my_ifa_list->ifa_address >> 8) & 0xFF;
> -				ipaddress[2] = (my_ifa_list->ifa_address >> 16) & 0xFF;
> -				ipaddress[3] = my_ifa_list->ifa_address >> 24;
>  				DBG_871X("%s: %d.%d.%d.%d ==========\n", __func__,
>  						ipaddress[0], ipaddress[1], ipaddress[2], ipaddress[3]);
>  				memcpy(pcurrentip, ipaddress, 4);
> diff -u -p ./drivers/staging/typec/fusb302/fusb302.c /tmp/nothing/drivers/staging/typec/fusb302/fusb302.c
> --- ./drivers/staging/typec/fusb302/fusb302.c
> +++ /tmp/nothing/drivers/staging/typec/fusb302/fusb302.c
> @@ -1040,7 +1040,6 @@ static int fusb302_pd_send_message(struc
>  	/* packsym tells the FUSB302 chip that the next X bytes are payload */
>  	buf[pos++] = FUSB302_TKN_PACKSYM | (len & 0x1F);
>  	buf[pos++] = msg->header & 0xFF;
> -	buf[pos++] = (msg->header >> 8) & 0xFF;
>
>  	len -= 2;
>  	memcpy(&buf[pos], msg->payload, len);
> Segmentation fault (core dumped)
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

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 <joe@perches.com> 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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Julia Lawall]

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 <joe@perches.com> 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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall <julia.lawall@lip6.fr> 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 <joe@perches.com> 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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Julia Lawall]

On Mon, 26 Jun 2017, Frans Klaver wrote:

> On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall <julia.lawall@lip6.fr> 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 <joe@perches.com> 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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

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 <joe@perches.com> 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

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Julia Lawall]

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 <joe@perches.com> 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