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

Message-ID: <20201109145025.GB3913@localhost.localdomain>
Date:   Mon, 9 Nov 2020 11:50:25 -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 03:24:37PM +0200, Vlad Buslov wrote:
> On Sun 08 Nov 2020 at 01:30, wenxu@...oud.cn wrote:
...
> > @@ -974,9 +974,22 @@ config NET_ACT_TUNNEL_KEY
> >  	  To compile this code as a module, choose M here: the
> >  	  module will be called act_tunnel_key.
> >  
> > +config NET_ACT_FRAG
> > +	tristate "Packet fragmentation"
> > +	depends on NET_CLS_ACT
> > +	help
> > +         Say Y here to allow fragmenting big packets when outputting
> > +         with the mirred action.
> > +
> > +	  If unsure, say N.
> > +
> > +	  To compile this code as a module, choose M here: the
> > +	  module will be called act_frag.
> > +
> 
> Just wondering, what is the motivation for putting the frag code into
> standalone module? It doesn't implement usual act_* interface and is not
> user-configurable. To me it looks like functionality that belongs to
> act_api. Am I missing something?

It's the way we found so far for not "polluting" mirred/tc with L3
functionality, per Cong's feedbacks on previous attempts. As for why
not act_api, this is not some code that other actions can just re-use
and that file is already quite big, so I thought act_frag would be
better to keep it isolated/contained.

If act_frag is confusing, then maybe act_mirred_frag? It is a mirred
plugin now, after all.

...
> > +int tcf_set_xmit_hook(int (*xmit_hook)(struct sk_buff *skb,
> > +				       int (*xmit)(struct sk_buff *skb)))
> > +{
> > +	if (!tcf_xmit_hook_enabled())
> > +		xchg(&tcf_xmit_hook, xmit_hook);
> 
> Marcelo, why did you suggest to use atomic operations to change
> tcf_xmit_hook variable? It is not obvious to me after reading the code.

I thought as a minimal way to not have problems on module removal, but
your comment below proves it is not right/enough. :-)

> 
> > +	else if (xmit_hook != tcf_xmit_hook)
> > +		return -EBUSY;
> > +
> > +	tcf_inc_xmit_hook();
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_set_xmit_hook);
> > +
> > +void tcf_clear_xmit_hook(void)
> > +{
> > +	tcf_dec_xmit_hook();
> > +
> > +	if (!tcf_xmit_hook_enabled())
> > +		xchg(&tcf_xmit_hook, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_clear_xmit_hook);
> > +
> > +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.

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?

> 
> > +		return tcf_xmit_hook(skb, xmit);
> > +	else
> > +		return xmit(skb);
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);