Mailing List Archive: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

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

Jul 30, 2021, 12:15 PM

Post #1 of 11 (2 views)

Fix sparse warnings of different base types in assignments
and in passing function parameters.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
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];
+ } 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);
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;
+ addr.f0 = be16_to_cpu(*ipxSocketAddr);
+ memcpy(networkAddr+5, addr.f1, 2);
}

static inline void __nat25_generate_apple_network_addr(unsigned char *networkAddr,
- unsigned short *network, unsigned char *node)
+ __be16 *network, unsigned char *node)
{
+ union {
+ unsigned short f0;
+ unsigned char f1[2];
+ } addr;
+
memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);

networkAddr[0] = NAT25_APPLE;
- memcpy(networkAddr+1, (unsigned char *)network, 2);
+ addr.f0 = be16_to_cpu(*network);
+ memcpy(networkAddr+1, addr.f1, 2);
networkAddr[3] = *node;
}

static inline void __nat25_generate_pppoe_network_addr(unsigned char *networkAddr,
- unsigned char *ac_mac, unsigned short *sid)
+ unsigned char *ac_mac, __be16 *sid)
{
+ union {
+ unsigned short f0;
+ unsigned char f1[2];
+ } addr;
+
memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);

networkAddr[0] = NAT25_PPPOE;
- memcpy(networkAddr+1, (unsigned char *)sid, 2);
+ addr.f0 = be16_to_cpu(*sid);
+ memcpy(networkAddr+1, addr.f1, 2);
memcpy(networkAddr+3, (unsigned char *)ac_mac, 6);
}

--
2.32.0

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

dan.carpenter at oracle

Aug 2, 2021, 8:15 AM

Post #2 of 11 (2 views)

Permalink

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?

>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 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?

> + } 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.

> 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????

> + addr.f0 = be16_to_cpu(*ipxSocketAddr);

I'm so puzzled.

> + memcpy(networkAddr+5, addr.f1, 2);

This patch is really weird so I'm done reviewing it.

regards,
dan carpenter

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

fmdefrancesco at gmail

Aug 2, 2021, 8:15 AM

Post #3 of 11 (2 views)

Permalink

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

fmdefrancesco at gmail

Aug 2, 2021, 10:15 AM

Post #4 of 11 (2 views)

Permalink

On Monday, August 2, 2021 4:26:33 PM CEST Fabio M. De Francesco wrote:
> 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.
> >
> > [...]
> > >
> > > + union {
> > > + unsigned int f0;
> > > + unsigned char f1[IPX_NODE_LEN];
> >
> > What is going on here?? Why is f1 six bytes?
>
> Please look at the third parameter of the latest memcpy() in this function.
>

No, I'm wrong here. I must have exchanged in my mind the latest and the
memcpy() before the latest. So I see a '6' in the wrong memcpy().

I'll fix it ASAP.

Thanks,

Fabio

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

fmdefrancesco at gmail

Aug 3, 2021, 2:15 AM

Post #5 of 11 (2 views)

Permalink

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@gmail.com>
> > ---
> >
> > 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

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

dan.carpenter at oracle

Aug 3, 2021, 8:15 AM

Post #6 of 11 (2 views)

Permalink

1) I'm pretty sure after a little more review that the patch introduces
bugs. The data is supposed to be big endian.

2) The commit message did not clearly describe the "bug" and how the it
looks like to the user.

3) The way we set variables to zero is we say "foo = 0;" We do not
say "foo ^= foo;". Everyone knows the XOR a variable with itself
trick but obfuscated code makes us cross.

4) The unions were unnecessary and ugly/bad.

regards,
dan carpenter

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

dan.carpenter at oracle

Aug 4, 2021, 2:15 AM

Post #7 of 11 (2 views)

Permalink

On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> 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;
> + addr.f0 = be16_to_cpu(*ipxSocketAddr);
> + memcpy(networkAddr+5, addr.f1, 2);

Here is another bug which was obscured/caused by the union.

addr.f0 = be16_to_cpu(*ipxSocketAddr);

The addr.f0 variable is an int. On big endian systems only the last two
bytes are set:

memcpy(networkAddr+5, addr.f1, 2);

So this is the equivalent of:

memset(networkAddr+5, 0, 2);

regards,
dan carpenter

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

fmdefrancesco at gmail

Aug 4, 2021, 2:15 AM

Post #8 of 11 (2 views)

Permalink

On Wednesday, August 4, 2021 9:59:30 AM CEST Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> > static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned
char
> > *networkAddr,>
> > - unsigned int *ipxNetAddr,
unsigned short *ipxSocketAddr)
> >
> > [...]
> >
> Here is another bug which was obscured/caused by the union.
>
> addr.f0 = be16_to_cpu(*ipxSocketAddr);
>
> The addr.f0 variable is an int. On big endian systems only the last two
> bytes are set:
>
> memcpy(networkAddr+5, addr.f1, 2);
>
> So this is the equivalent of:
>
> memset(networkAddr+5, 0, 2);
>
> regards,
> dan carpenter

Dear Dan,

Thanks, for pointing me to one more bug I introduced with this patch. The most
of them were due to me forgetting that memcpy() takes pointers. For some
reason I was thinking it takes values, therefore I put in it a lot of
unnecessary and faulty complications.

I'd like to make a new patch, a better one (I hope), without unneeded unions
without the other wrong lines that are in the commit 56febcc2595e.

However, I see that Greg hasn't yet had the time to revert the above commit,
so I don't know how to make a new patch.

I mean: I could (1) either wait for Greg to revert it and then to fix the
sparse warnings with a new patch, or (2) I could fix the bugs I made in
56febcc2595e without having it reverted. I would prefer the solution (2) with
a "Fixes: 56febcc2595e (...)" and a "Reported-by: Dan Carpenter <...>" tags.

What is the best solution between the two above?

Thanks,

Fabio

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

dan.carpenter at oracle

Aug 4, 2021, 4:15 AM

Post #9 of 11 (2 views)

Permalink

The patch was based on the faulty premise that the original code was
buggy so I don't think it can be fixed. It just needs to be reverted.

regards,
dan carpenter

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

fmdefrancesco at gmail

Aug 4, 2021, 6:15 AM

Post #10 of 11 (2 views)

Permalink

On Wednesday, August 4, 2021 11:58:39 AM CEST Dan Carpenter wrote:
> The patch was based on the faulty premise that the original code was
> buggy so I don't think it can be fixed. It just needs to be reverted.
>
> regards,
> dan carpenter

With the original code, GCC + sparse emit a dozen of warnings like the
following ones:

drivers/staging/r8188eu/core/rtw_br_ext.c:693:101: warning: incorrect type in
argument 2 (different base types)
drivers/staging/r8188eu/core/rtw_br_ext.c:693:101: expected unsigned int
*ipxNetAddr
drivers/staging/r8188eu/core/rtw_br_ext.c:693:101: got restricted __be32 *
drivers/staging/r8188eu/core/rtw_br_ext.c:693:123: warning: incorrect type in
argument 3 (different base types)
drivers/staging/r8188eu/core/rtw_br_ext.c:693:123: expected unsigned short
*ipxSocketAddr
drivers/staging/r8188eu/core/rtw_br_ext.c:693:123: got restricted __be16 *
drivers/staging/r8188eu/core/rtw_br_ext.c:698:99: warning: incorrect type in
argument 2 (different base types)

Do we want the above pasted warnings and leave the code as-is?

Regards,

Fabio

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

dan.carpenter at oracle

Aug 4, 2021, 6:15 AM

Post #11 of 11 (2 views)

Permalink

There is no reason to assume that the Sparse annotations are correct or
complete.

I'm sorry, but you're going to have to figure this one out on your own.
I showed 100% that your patch introduced bugs. That's the extent of my
job. I'm not going to answer any more questions.

regards,
dan carpenter