Re: [PATCH net] net/sched: act_mirred: fix fragment the packet after defrag in act_ct

From: Cong Wang <xiyou.wangcong@gmail.com>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: wenxu <wenxu@ucloud.cn>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>
Subject: Re: [PATCH net] net/sched: act_mirred: fix fragment the packet after defrag in act_ct
Date: Thu, 2 Jul 2020 14:39:07 -0700	[thread overview]
Message-ID: <CAM_iQpUrRzOi-S+49jMhDQCS0jqOmwObY3ZNa6n9qJGbPTXM-A@mail.gmail.com> (raw)
In-Reply-To: <20200702173228.GH74252@localhost.localdomain>

On Thu, Jul 2, 2020 at 10:32 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Thu, Jul 02, 2020 at 05:36:38PM +0800, wenxu wrote:
> >
> > On 7/2/2020 1:33 AM, Cong Wang wrote:
> > > On Wed, Jul 1, 2020 at 1:21 AM wenxu <wenxu@ucloud.cn> wrote:
> > >>
> > >> On 7/1/2020 2:21 PM, wenxu wrote:
> > >>> On 7/1/2020 2:12 PM, Cong Wang wrote:
> > >>>> On Tue, Jun 30, 2020 at 11:03 PM wenxu <wenxu@ucloud.cn> wrote:
> > >>>>> Only forward packet case need do fragment again and there is no need do defrag explicit.
> > >>>> Same question: why act_mirred? You have to explain why act_mirred
> > >>>> has the responsibility to do this job.
> > >>> The fragment behavior only depends on the mtu of the device sent in act_mirred. Only in
> > >>>
> > >>> the act_mirred can decides whether do the fragment or not.
> > >> Hi cong,
> > >>
> > >>
> > >> I still think this should be resolved in the act_mirred.  Maybe it is not matter with a "responsibility"
> > >>
> > >> Did you have some other suggestion to solve this problem?
> > > Like I said, why not introduce a new action to handle fragment/defragment?
> > >
> > > With that, you can still pipe it to act_ct and act_mirred to achieve
> > > the same goal.
> >
> > Thanks.  Consider about the act_fagment, There are two problem for this.
> >
> >
> > The frag action will put the ressemble skb to more than one packets. How these packets
> >
> > go through the following tc filter or chain?
>
> One idea is to listificate it, but I don't see how it can work. For
> example, it can be quite an issue when jumping chains, as the match
> would have to work on the list as well.

Why is this an issue? We already use goto action for use cases like
vlan pop/push. The header can be changed all the time, reclassification
is necessary.

>
> >
> >
> > When should use the act_fragament the action,  always before the act_mirred?
>
> Which can be messy if you consider chains like: "mirred, push vlan,
> mirred" or so. "frag, mirred, defrag, push vlan, frag, mirred".

So you mean we should have a giant act_do_everything?

"Do one thing do it well" is exactly the philosophy of designing tc
actions, if you are against this, you are too late in the game.

Thanks.