netdev - Re: [net-next PATCH V1 3/3] net: warn on napi_alloc

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: <CAKgT0UcJyxDvY+r6qsXSCQ4SjE2VH4NPGOrG9F7Ame50cQQpKA@mail.gmail.com>
Date:	Mon, 9 May 2016 09:33:25 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Jesper Dangaard Brouer <brouer@...hat.com>
Cc:	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Or Gerlitz <gerlitz.or@...il.com>,
	Eugenia Emantayev <eugenia@...lanox.com>
Subject: Re: [net-next PATCH V1 3/3] net: warn on napi_alloc_skb being called
 in wrong context

On Mon, May 9, 2016 at 6:44 AM, Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
> It have always been required to call napi_alloc_skb from NAPI/softirq
> context, which implies running with local_bh_disable'ed.  Thus, this
> code path should already be well tested. But recent SKB bulk changes
> introduced will make this more volatile and bugs more subtle, if this
> is violated.

It hasn't been required to be in the softirq routine, bottom halves
could have been disabled.  I used the NAPI parameter to try and
enforce a strong correlation between the two but I didn't make it a
hard requirement.  It isn't until you try to do the recycling that it
becomes more of a requirement because then it is tied into the bulk
free.

The bulk free routine had a stronger requirement for NAPI because it
needed to have the __kfree_skb_flush routine called to finally free
the skbuffs that were still hanging off the percpu skbuff structure.

> To catch any driver violating this add a loud WARN_ON.
>
> Performance wise, I do worry about adding this runtime check code into
> the hotpath, of this highly optimized function call.  I've
> micro-benchmarked it with both IP-forwarding and local UDP delivery,
> and didn't see any regressions.  It does adds extra code size (icache).
>
> add/remove: 0/0 grow/shrink: 1/0 up/down: 43/0 (43)
> function                                     old     new   delta
> __napi_alloc_skb                             461     504     +43
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  net/core/skbuff.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index e85f1065b263..99addbf66f2e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -498,6 +498,9 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
>         struct sk_buff *skb;
>         void *data;
>
> +       /* Catch drivers violating, not having local BH disabled */
> +       WARN_ON(!in_softirq());
> +

I'm pretty sure this still lets you get away with just disabling
bottom halves.  I think what you would want is in_serving_softirq().

>         len += NET_SKB_PAD + NET_IP_ALIGN;
>
>         if (unlikely((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
>