netdev - Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAjsZQwKbp-3QgBj9KEUoqLvaE5pLX8wsLq01TDC8HdVp=8pLg@mail.gmail.com>
Date: Sat, 3 Aug 2024 10:47:35 +0900
From: Moon Yeounsu <yyyynoom@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: cooldavid@...ldavid.org, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

On Fri, Aug 2, 2024 at 11:15 PM Simon Horman <horms@...nel.org> wrote:
>
> On Fri, Aug 02, 2024 at 02:44:21PM +0900, Moon Yeounsu wrote:
> > `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> > Therefore, we should use a well-defined function not a bit shift
> > to find the header length.
> >
> > It also compress two lines at a single line.
> >
> > Signed-off-by: Moon Yeounsu <yyyynoom@...il.com>
>
> Firstly, I think this clean-up is both correct and safe.  Safe because
> ip_hdrlen() only relies on ip_hdr(), which is already used in the same code
> path. And correct because ip_hdrlen multiplies ihl by 4, which is clearly
> equivalent to a left shift of 2 bits.
Firstly, Thank you for reviewing my patch!
>
> However, I do wonder about the value of clean-ups for what appears to be a
> very old driver, which hasn't received a new feature for quite sometime
Oh, I don't know that...
>
> And further, I wonder if we should update this driver from "Maintained" to
> "Odd Fixes" as the maintainer, "Guo-Fu Tseng" <cooldavid@...ldavid.org>,
> doesn't seem to have been seen by lore since early 2020.
>
> https://lore.kernel.org/netdev/20200219034801.M31679@cooldavid.org/
Then, how about deleting the file from the kernel if the driver isn't
maintained?
Many people think like that (At least I think so)
There are files, and if there are issues, then have to fix them.
Who can think unmanaged files remain in the kernel?

> > ---
> >  drivers/net/ethernet/jme.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> > index b06e24562973..83b185c995df 100644
> > --- a/drivers/net/ethernet/jme.c
> > +++ b/drivers/net/ethernet/jme.c
> > @@ -946,15 +946,13 @@ jme_udpsum(struct sk_buff *skb)
> >       if (skb->protocol != htons(ETH_P_IP))
> >               return csum;
> >       skb_set_network_header(skb, ETH_HLEN);
> > +
> >       if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
> > -         (skb->len < (ETH_HLEN +
> > -                     (ip_hdr(skb)->ihl << 2) +
> > -                     sizeof(struct udphdr)))) {
> > +         (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {
>
> The parentheses around the call to ip_hdrlen are unnecessary.
> And this line is now too long: networking codes till prefers
> code to be 80 columns wide or less.
Okay, I'll keep the kernel coding style too!
>
> >               skb_reset_network_header(skb);
> >               return csum;
> >       }
> > -     skb_set_transport_header(skb,
> > -                     ETH_HLEN + (ip_hdr(skb)->ihl << 2));
> > +     skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));
>
> Unnecessary parentheses here too.
Also fix it :)
>
> >       csum = udp_hdr(skb)->check;
> >       skb_reset_transport_header(skb);
> >       skb_reset_network_header(skb);
>
> --
> pw-bot: cr

Thank you for paying attention to my patch! I'm a beginner who just
came to the kernel.
So... Sorry if I sounded presumptuous and didn't know much about kernels.
But I don't understand why we have to pay attention to unmanaged kernel files.
And why do we have to check whether the file is managed or not?

Thank you for reading my email ^오^