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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.