netdev - Re: [PATCH net-next v2 0/3] make nf_ct_frag/6

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: <20200715211714.GR32005@breakpoint.cc>
Date:   Wed, 15 Jul 2020 23:17:14 +0200
From:   Florian Westphal <fw@...len.de>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     wenxu@...oud.cn, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, fw@...len.de,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH net-next v2 0/3] make nf_ct_frag/6_gather elide the skb
 CB clear

Jakub Kicinski <kuba@...nel.org> wrote:
> On Tue,  7 Jul 2020 12:55:08 +0800 wenxu@...oud.cn wrote:
> > From: wenxu <wenxu@...oud.cn>
> > 
> > Add nf_ct_frag_gather and Make nf_ct_frag6_gather elide the CB clear 
> > when packets are defragmented by connection tracking. This can make
> > each subsystem such as br_netfilter, openvswitch, act_ct do defrag
> > without restore the CB. 
> > This also avoid serious crashes and problems in  ct subsystem.
> > Because Some packet schedulers store pointers in the qdisc CB private
> > area and parallel accesses to the SKB.
> > 
> > This series following up
> > http://patchwork.ozlabs.org/project/netdev/patch/1593422178-26949-1-git-send-email-wenxu@ucloud.cn/
> > 
> > patch1: add nf_ct_frag_gather elide the CB clear
> > patch2: make nf_ct_frag6_gather elide the CB clear
> > patch3: fix clobber qdisc_skb_cb in act_ct with defrag
> > 
> > v2: resue some ip_defrag function in patch1
> 
> Florian, Cong - are you willing to venture an ack on these? Anyone?

Nope, sorry.  Reason is that I can't figure out the need for this series.
Taking a huge step back:

http://patchwork.ozlabs.org/project/netdev/patch/1593422178-26949-1-git-send-email-wenxu@ucloud.cn/

That patch looks ok to me:
I understand the problem statement/commit message and I can see how its addressed.

I don't understand why the CB clearing must be avoided.

defrag assumes skb ownership -- e.g. it may realloc skb->data
(calls pskb_may_pull), it calls skb_orphan(), etc.

AFAICS, tcf_classify makes same assumption -- exclusive ownership
and no parallel skb accesses.

So, if in fact the "only" problem is the loss of
qdisc_skb_cb(skb)->pkt_len, then the other patch looks ok to me.

If we indeed have parallel access, then I do not understand how
avoiding the memsets in the defrag path makes things any better
(see above wrt. skb pull and the like).

As for these patches here:

- if (!(IPCB(skb)->flags & IPSKB_FRAG_COMPLETE) &&
+ if ((ignore_skb_cb || !(IPCB(skb)->flags & IPSKB_FRAG_COMPLETE)) &&

This is very questionable, we take different code path depending
on call site.

Why is it okay to unconditionally take this branch for act_ct case (ignore_skb_cb set)?