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