Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

On Monday, August 2, 2021 4:05:05 PM CEST Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> > Fix sparse warnings of different base types in assignments
> > and in passing function parameters.
> 
> This patch fixes some endian bugs but it's not mentioned at all in the
> commit message.  Did you send to the correct patch?
Dear Greg, Dan,

I just read another message where Greg K-H decided to revert this patch, 
probably also according to the review by Dan C..

In doing so, the sparse warnings of different base types in assignments and 
some endian bugs will appear anew and I think that these problem should be 
addressed in some other way.

I'll try to find some other solutions that result to be more correct and in 
line with Linux conventions. But, before doing the work I need to better 
understand what went wrong the first time. So, please, devote some time to 
respond to my questions...

> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> > ---
> > 
> >  drivers/staging/r8188eu/core/rtw_br_ext.c | 46 ++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > b/drivers/staging/r8188eu/core/rtw_br_ext.c index 
e00302137a60..31ca2e548555 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -71,7 +71,7 @@ static inline int __nat25_add_pppoe_tag(struct sk_buff 
*skb, struct
> > pppoe_tag *t> 
> >  	struct pppoe_hdr *ph = (struct pppoe_hdr *)(skb->data + ETH_HLEN);
> >  	int data_len;
> > 
> > -	data_len = tag->tag_len + TAG_HDR_LEN;
> > +	data_len = be16_to_cpu(tag->tag_len) + TAG_HDR_LEN;
> > 
> >  	if (skb_tailroom(skb) < data_len) {
> >  	
> >  		_DEBUG_ERR("skb_tailroom() failed in add SID tag!\n");
> >  		return -1;
> > 
> > @@ -134,42 +134,68 @@ static inline void 
__nat25_generate_ipv4_network_addr(unsigned
> > char *networkAddr> 
> >  }
> >  
> >  static inline void __nat25_generate_ipx_network_addr_with_node(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned char *ipxNodeAddr)
> > +				__be32 *ipxNetAddr, unsigned char 
*ipxNodeAddr)
> > 
> >  {
> > 
> > +	union {
> > +                unsigned int f0;
> > +                unsigned char f1[IPX_NODE_LEN];
> 
> What is going on here??  Why is f1 six bytes?

Here I've been misled by something else. I agree that f1 should be four bytes.
 
> > +        } addr;
> > +
> > 
> >  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
> >  	
> >  	networkAddr[0] = NAT25_IPX;
> > 
> > -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> > +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> > +	memcpy(networkAddr+1, addr.f1, 4);
> 
> What's the point of a union?  memcpy() doesn't care about endian
> anotations.

I'm not sure to understand. Please, elaborate a bit. Do you mean that there's 
no need to use be32_to_cpu(*ipxNetAddr) and that we can use ipxNetAddr as-is 
(it is a __be32) as the second parameter of memcpy(), even though the 
networkAddr+1 is the address of an array of unsigned char? Is this that you 
tried to explain to me?

However, there's a huge error in passing the second parameter to memcpy(): it 
should be &addr.f1. Or better, let's make addr to be __be32 and then pass 
&addr (so there's no more need of that union). Do you agree?

> >  	memcpy(networkAddr+5, ipxNodeAddr, 6);
> >  
> >  }
> >  
> >  static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned short *ipxSocketAddr)
> > +				__be32 *ipxNetAddr, __be16 
*ipxSocketAddr)
> > 
> >  {
> > 
> > +	union {
> > +		unsigned int f0;
> > +		unsigned char f1[4];
> > +	} addr;
> > +
> > 
> >  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
> >  	
> >  	networkAddr[0] = NAT25_IPX;
> > 
> > -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> > -	memcpy(networkAddr+5, (unsigned char *)ipxSocketAddr, 2);
> > +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> > +	memcpy(networkAddr+1, addr.f1, 4);
> > +	addr.f0 ^= addr.f0;
> 
> What on earth????

According to my understanding I need to wipe all the bits of addr.f0 before 
reusing it in the next assignment. Don't I have to?

If we agree on this, I'd like to explain why I use the xor of itself... I know 
that `addr.f0 = 0;` is more easily readable. But I've always thought that a 
direct assignment of a specific value (0, in this case) should indicate that 
one wants to use that 0 for something that needs that specific number.

For example, in `for(i = 0; i < len; i++)` to indicate that we want to loop 
starting from 0; or in `res = some_func(); if( !res ) return _FAIL;`, and so 
on. In this cases I want to make it clear that 0 has a precise and special 
meaning.

Differently, when one simply wants a variable wiped out of any content, just 
before re-using it for a subsequent "real" assignment, I prefer to xor(ing) 
that variable with itself.

Does it make sense to you?

> > +	addr.f0 = be16_to_cpu(*ipxSocketAddr);
> 
> I'm so puzzled.
>

Again like the assignment above, *ipxSocketAddr is of type __be16 and the 
destination of memcpy() is an array of unsigned char(s). I think I could just 
have a __be16 variable and pass the address of it to memcpy.

Do you agree with the solution above? (Probably I was so distracted that I 
didn't see I should have passed a pointer as the second parameter of 
memcpy()).
 
> > +	memcpy(networkAddr+5, addr.f1, 2);
> 
> This patch is really weird so I'm done reviewing it.
> 
> regards,
> dan carpenter

I'm going to wait a bit for your reply to this message. If I don't receive any 
response I'll wait for the revert and make a new patch and see what happens.
Or, I could fix the patch, if Greg does not revert 56febcc2595e.

Patiently waiting... :-)

Thanks,

Fabio