Message-ID: <5ab182b6-6ac7-16f7-7eae-7001be2b6da7@iogearbox.net>
Date: Fri, 27 Oct 2023 20:21:40 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: kuba@...nel.org, idosch@...sch.org, netdev@...r.kernel.org,
 bpf@...r.kernel.org
Subject: Re: [PATCH net-next] net, sched: Fix SKB_NOT_DROPPED_YET splat under
 debug config

On 10/27/23 7:24 PM, Jamal Hadi Salim wrote:
> On Fri, Oct 27, 2023 at 9:51 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>>
>> Ido reported:
>>
>>    [...] getting the following splat [1] with CONFIG_DEBUG_NET=y and this
>>    reproducer [2]. Problem seems to be that classifiers clear 'struct
>>    tcf_result::drop_reason', thereby triggering the warning in
>>    __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). [...]
>>
>>    [1]
>>    WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130
>>    Modules linked in:
>>    CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682
>>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014
>>    RIP: 0010:kfree_skb_reason+0x38/0x130
>>    [...]
>>    Call Trace:
>>     <IRQ>
>>     __netif_receive_skb_core.constprop.0+0x837/0xdb0
>>     __netif_receive_skb_one_core+0x3c/0x70
>>     process_backlog+0x95/0x130
>>     __napi_poll+0x25/0x1b0
>>     net_rx_action+0x29b/0x310
>>     __do_softirq+0xc0/0x29b
>>     do_softirq+0x43/0x60
>>     </IRQ>
>>
>>    [2]
>>    #!/bin/bash
>>
>>    ip link add name veth0 type veth peer name veth1
>>    ip link set dev veth0 up
>>    ip link set dev veth1 up
>>    tc qdisc add dev veth1 clsact
>>    tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop
>>    mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1
>>
>> What happens is that inside most classifiers the tcf_result is copied over
>> from a filter template e.g. *res = f->res which then implicitly overrides
>> the prior SKB_DROP_REASON_TC_{INGRESS,EGRESS} default drop code which was
>> set via sch_handle_{ingress,egress}() for kfree_skb_reason().
>>
>> Add a small helper tcf_set_result() and convert classifiers over to it.
>> The latter leaves the drop code intact and classifiers, actions as well
>> as the action engine in tcf_exts_exec() can then in future make use of
>> tcf_set_drop_reason(), too.
>>
>> Tested that the splat is fixed under CONFIG_DEBUG_NET=y with the repro.
>>
>> Fixes: 54a59aed395c ("net, sched: Make tc-related drop reason more flexible")
>> Reported-by: Ido Schimmel <idosch@...sch.org>
>> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
>> Cc: Jamal Hadi Salim <jhs@...atatu.com>
>> Cc: Jakub Kicinski <kuba@...nel.org>
>> Link: https://lore.kernel.org/netdev/ZTjY959R+AFXf3Xy@shredder
>> ---
>>   include/net/pkt_cls.h    | 12 ++++++++++++
>>   net/sched/cls_basic.c    |  2 +-
>>   net/sched/cls_bpf.c      |  2 +-
>>   net/sched/cls_flower.c   |  2 +-
>>   net/sched/cls_fw.c       |  2 +-
>>   net/sched/cls_matchall.c |  2 +-
>>   net/sched/cls_route.c    |  4 ++--
>>   net/sched/cls_u32.c      |  2 +-
>>   8 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index a76c9171db0e..31d8e8587824 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -160,6 +160,18 @@ static inline void tcf_set_drop_reason(struct tcf_result *res,
>>          res->drop_reason = reason;
>>   }
>>
>> +static inline void tcf_set_result(struct tcf_result *to,
>> +                                 const struct tcf_result *from)
>> +{
>> +       /* tcf_result's drop_reason which is the last member must be
>> +        * preserved and cannot be copied from the cls'es tcf_result
>> +        * template given this is carried all the way and potentially
>> +        * set to a concrete tc drop reason upon error or intentional
>> +        * drop. See tcf_set_drop_reason() locations.
>> +        */
>> +       memcpy(to, from, offsetof(typeof(*to), drop_reason));
>> +}
> 
> I believe our bigger issue here is we are using this struct now for
> both policy set by the control plane and for runtime decisions

Hm, but that was also either way in the original rfc.

> (drop_reason) - whereas the original assumption was this struct only
> held set policy. In retrospect we should have put the verdict(which is
> policy) here and return the error code (as was in the first patch). I
> am also not sure humans would not make a mistake on "this field must
> be at the end of the struct". Can we put some assert (or big comment
> on the struct) to make sure someone does not overwrite this field?

Yeah that can be done.

> Also what happens if "from" above has a set drop_reason - is that
> lost? Do you need an assert there as well?

Why it's needed, do you have a use case for it?

> BTW: The simple patch i posted fixes the problem as well (i actually
> tested it minus the typo i sent).

It didn't compile for me, but if you think it's a better approach, yes,
feel free to post it as a proper patch then.

What I'm not quite following though is, I thought your original use case
was that you want to be able to troubleshoot drops from unexpected
locations (aka not policy) in the tc engine so won't this miss cases when
you would then want to use tcf_set_drop_reason() e.g. from tcf_action_exec()
upon 'exception' cases (like the one for example I pointed out)? With the
diff you proposed it will basically fallback to SKB_DROP_REASON_TC_{INGRESS,
EGRESS}, so override anything that would have been set from there.

Thanks,
Daniel

Powered by blists - more mailing lists