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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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@gmail.com> wrote:
> > On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
> >> On Sun 08 Nov 2020 at 01:30, wenxu@ucloud.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);
>