From: Florian Westphal <fw@strlen.de>
To: Jakub Kicinski <kuba@kernel.org>
Cc: wenxu@ucloud.cn, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org, fw@strlen.de,
Cong Wang <xiyou.wangcong@gmail.com>
Subject: Re: [PATCH net-next v2 0/3] make nf_ct_frag/6_gather elide the skb CB clear
Date: Wed, 15 Jul 2020 23:17:14 +0200 [thread overview]
Message-ID: <20200715211714.GR32005@breakpoint.cc> (raw)
In-Reply-To: <20200715132659.34fa0e14@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 7 Jul 2020 12:55:08 +0800 wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.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)?
next prev parent reply other threads:[~2020-07-15 21:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-07 4:55 [PATCH net-next v2 0/3] make nf_ct_frag/6_gather elide the skb CB clear wenxu
2020-07-07 4:55 ` [PATCH net-next v2 1/3] net: ip_fragment: Add ip_defrag_ignore_cb support wenxu
2020-07-07 4:55 ` [PATCH net-next v2 2/3] netfilter: nf_conntrack_reasm: make nf_ct_frag6_gather elide the CB clear wenxu
2020-07-07 4:55 ` [PATCH net-next v2 3/3] net/sched: act_ct: fix clobber qdisc_skb_cb in defrag wenxu
2020-07-15 20:26 ` [PATCH net-next v2 0/3] make nf_ct_frag/6_gather elide the skb CB clear Jakub Kicinski
2020-07-15 21:17 ` Florian Westphal [this message]
2020-07-16 2:42 ` wenxu
2020-07-17 15:32 ` Cong Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200715211714.GR32005@breakpoint.cc \
--to=fw@strlen.de \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=wenxu@ucloud.cn \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).