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] [day] [month] [year] [list]
Message-ID: <948c5844-b8e4-81d3-b5eb-b13b1d3cda38@solarflare.com>
Date:   Thu, 25 Jun 2020 15:25:38 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Alexander Lobakin <alobakin@...nk.ru>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Ido Schimmel <idosch@...lanox.com>,
        "Paolo Abeni" <pabeni@...hat.com>,
        Petr Machata <petrm@...lanox.com>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 net-next] net: core: use listified Rx for GRO_NORMAL in
 napi_gro_receive()

On 24/06/2020 22:06, Jason A. Donenfeld wrote:
> Hi Alexander,
>
> This patch introduced a behavior change around GRO_DROP:
>
> napi_skb_finish used to sometimes return GRO_DROP:
>
>> -static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
>> +static gro_result_t napi_skb_finish(struct napi_struct *napi,
>> +				    struct sk_buff *skb,
>> +				    gro_result_t ret)
>>  {
>>  	switch (ret) {
>>  	case GRO_NORMAL:
>> -		if (netif_receive_skb_internal(skb))
>> -			ret = GRO_DROP;
>> +		gro_normal_one(napi, skb);
>>
> But under your change, gro_normal_one and the various calls that makes
> never propagates its return value, and so GRO_DROP is never returned to
> the caller, even if something drops it.
This followed the pattern set by napi_frags_finish(), and is
 intentional: gro_normal_one() usually defers processing of
 the skb to the end of the napi poll, so by the time we know
 that the network stack has dropped it, the caller has long
 since returned.
In fact the RX will be handled by netif_receive_skb_list_internal(),
 which can't return NET_RX_SUCCESS vs. NET_RX_DROP, because it's
 handling many skbs which might not all have the same verdict.

When originally doing this work I felt this was OK because
 almost no-one was sensitive to the return value — almost the
 only callers that were were in our own sfc driver, and then
 only for making bogus decisions about interrupt moderation.
Alexander just followed my lead, so don't blame him ;-)

> For some context, I'm consequently mulling over this change in my code,
> since checking for GRO_DROP now constitutes dead code:
Incidentally, it's only dead because dev_gro_receive() can't
 return GRO_DROP either.  If it could, napi_skb_finish()
 would pass that on.  And napi_gro_frags() (which AIUI is the
 better API for some performance reasons that I can't remember)
 can still return GRO_DROP too.

However, I think that incrementing your rx_dropped stat when
 the network stack chose to drop the packet is the wrong
 thing to do anyway (IMHO rx_dropped is for "there was a
 packet on the wire but either the hardware or the driver was
 unable to receive it"), so I'd say go ahead and remove the
 check.

HTH
-ed

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ