netdev - Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.

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] [day] [month] [year] [list]

Message-ID: <20201109164400.GC3913@localhost.localdomain>
Date:   Mon, 9 Nov 2020 13:44:00 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Vlad Buslov <vladbu@...dia.com>
Cc:     wenxu@...oud.cn, kuba@...nel.org, dcaratti@...hat.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet
 fragment support.

On Mon, Nov 09, 2020 at 05:47:46PM +0200, Vlad Buslov wrote:
> 
> On Mon 09 Nov 2020 at 16:50, Marcelo Ricardo Leitner <marcelo.leitner@...il.com> wrote:
> > On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
> >> On Sun 08 Nov 2020 at 01:30, wenxu@...oud.cn wrote:
...
> >> > +int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
> >> > +{
> >> > +	if (tcf_xmit_hook_enabled())
> >> 
> >> Okay, so what happens here if tcf_xmit_hook is disabled concurrently? If
> >> we get here from some rule that doesn't involve act_ct but uses
> >> act_mirred and act_ct is concurrently removed decrementing last
> >> reference to static branch and setting tcf_xmit_hook to NULL?
> >
> > Yeah.. good point. Thinking further now, what about using RCU for the
> > hook? AFAICT it can cover the synchronization needed when clearing the
> > pointer, tcf_set_xmit_hook() should do a module_get() and
> > tcf_clear_xmit_hook() can delay a module_put(act_frag) as needed with
> > call_rcu.
> 
> Wouldn't it be enough to just call synchronize_rcu() in
> tcf_clear_xmit_hook() after setting tcf_xmit_hook to NULL? act_ct module
> removal should be very rare, so synchronously waiting for rcu grace
> period to complete is probably okay.

Right. And even if it gets reloaded (or, say, something else tries to
use the hook), the teardown was already handled. Nice, thanks Vlad.

> 
> >
> > I see tcf_mirred_act is already calling rcu_dereference_bh(), so
> > it's already protected by rcu read here and calling tcf_xmit_hook()
> > with xmit pointer should be fine. WDYT?
> 
> Yes, good idea.
> 
> >
> >> 
> >> > +		return tcf_xmit_hook(skb, xmit);
> >> > +	else
> >> > +		return xmit(skb);
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
>