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]

Date:   Thu, 16 Jul 2020 10:42:11 +0800
From:   wenxu <wenxu@...oud.cn>
To:     Florian Westphal <fw@...len.de>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org,
        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


On 7/16/2020 5:17 AM, Florian Westphal wrote:
> 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).

Hi David,


What case for the parallel access the skb in tcf_classify?

If there indeed have this. Maybe it can't do defrag which also

access and modify the skb?


BR

wenxu