netdev - Re: [RFC PATCH net] net-gro: restore check for NULL skb in napi_gro

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZM0oE98EpPgAilda@luigi.stachecki.net>
Date: Fri, 4 Aug 2023 12:32:19 -0400
From: Tyler Stachecki <stachecki.tyler@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: edward.cree@....com, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com, Edward Cree <ecree.xilinx@...il.com>,
	netdev@...r.kernel.org, Martin Habets <habetsm.xilinx@...il.com>
Subject: Re: [RFC PATCH net] net-gro: restore check for NULL skb in
 napi_gro_frags

On Fri, Aug 04, 2023 at 04:45:13PM +0200, Eric Dumazet wrote:
> This is already done, a compiler should already infer this from this code:
> 
> if (unlikely(skb_gro_header_hard(skb, hlen))) {
>     eth = skb_gro_header_slow(skb, hlen, 0);
>     if (unlikely(!eth)) {
>         net_warn_ratelimited("%s: dropping impossible skb from %s\n",
>                                            __func__, napi->dev->name);
>         napi_reuse_skb(napi, skb);
>         return NULL;
>     }

It is a good point - though, FWIW, at least with clang-16 I am observing that
it does not leverage this fact (fully?). Mostly for my own curiosity, I took a
look...

In both cases, the generated code for the NULL check is emitted as a a forward
branch, meaning that (at least on x86), the branch direction hint is rendered
as desired.

However, without unlikely(...), the code for the taken branch is clumped
roughly after the inlined code for napi_frags_finish and before the (hot paths
of) napi_frags_skb.

With unlikely added, the code for the taken NULL check is seated right next to
the code generated for the unlikely paths in your comment. So, it does seem to
have an effect, however minor!

---

Anyways: perhaps the more important note here is that, at least with clang-16,
I can confirm that everything is still inlined with this change.