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

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

[wenxu]

From: wenxu <wenxu@ucloud.cn>

The fragment packets do defrag in act_ct module. The reassembled packet
over the mtu in the act_mirred. This big packet should be fragmented
to send out.

Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
This patch is based on
http://patchwork.ozlabs.org/project/netdev/patch/1593422178-26949-1-git-send-email-wenxu@ucloud.cn/

 include/net/sch_generic.h |   6 +-
 net/sched/act_ct.c        |   7 ++-
 net/sched/act_mirred.c    | 157 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 158 insertions(+), 12 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c510b03..3597244 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -384,6 +384,7 @@ struct qdisc_skb_cb {
 	};
 #define QDISC_CB_PRIV_LEN 20
 	unsigned char		data[QDISC_CB_PRIV_LEN];
+	u16			mru;
 };
 
 typedef void tcf_chain_head_change_t(struct tcf_proto *tp_head, void *priv);
@@ -1283,9 +1284,4 @@ void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
 void mini_qdisc_pair_block_init(struct mini_Qdisc_pair *miniqp,
 				struct tcf_block *block);
 
-static inline int skb_tc_reinsert(struct sk_buff *skb, struct tcf_result *res)
-{
-	return res->ingress ? netif_receive_skb(skb) : dev_queue_xmit(skb);
-}
-
 #endif
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index ec0250f..59c85ee 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -705,6 +705,8 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
 		local_bh_enable();
 		if (err && err != -EINPROGRESS)
 			goto out_free;
+
+		cb.mru = IPCB(skb)->frag_max_size;
 	} else { /* NFPROTO_IPV6 */
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
 		enum ip6_defrag_users user = IP6_DEFRAG_CONNTRACK_IN + zone;
@@ -713,6 +715,8 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
 		err = nf_ct_frag6_gather(net, skb, user);
 		if (err && err != -EINPROGRESS)
 			goto out_free;
+
+		cb.mru = IP6CB(skb)->frag_max_size;
 #else
 		err = -EOPNOTSUPP;
 		goto out_free;
@@ -1017,7 +1021,8 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 
 out:
 	tcf_action_update_bstats(&c->common, skb);
-	qdisc_skb_cb(skb)->pkt_len = skb->len;
+	if (qdisc_skb_cb(skb)->mru)
+		qdisc_skb_cb(skb)->pkt_len = skb->len;
 	return retval;
 
 drop:
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 83dd82f..2ab9180 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -21,7 +21,11 @@
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/dst.h>
+#include <net/ip.h>
+#include <net/ip6_fib.h>
 #include <linux/tc_act/tc_mirred.h>
+#include <linux/netfilter_ipv6.h>
 #include <net/tc_act/tc_mirred.h>
 
 static LIST_HEAD(mirred_list);
@@ -30,6 +34,18 @@
 #define MIRRED_RECURSION_LIMIT    4
 static DEFINE_PER_CPU(unsigned int, mirred_rec_level);
 
+struct tcf_mirred_frag_data {
+	unsigned long dst;
+	struct qdisc_skb_cb cb;
+	__be16 inner_protocol;
+	u16 vlan_tci;
+	__be16 vlan_proto;
+	unsigned int l2_len;
+	u8 l2_data[VLAN_ETH_HLEN];
+};
+
+static DEFINE_PER_CPU(struct tcf_mirred_frag_data, tcf_mirred_frag_data_storage);
+
 static bool tcf_mirred_is_act_redirect(int action)
 {
 	return action == TCA_EGRESS_REDIR || action == TCA_INGRESS_REDIR;
@@ -207,6 +223,138 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	return err;
 }
 
+static int tcf_mirred_output(struct net *net, struct sock *sk, struct sk_buff *skb)
+{
+	struct tcf_mirred_frag_data *data = this_cpu_ptr(&tcf_mirred_frag_data_storage);
+
+	if (skb_cow_head(skb, data->l2_len) < 0) {
+		kfree_skb(skb);
+		return -ENOMEM;
+	}
+
+	__skb_dst_copy(skb, data->dst);
+	*qdisc_skb_cb(skb) = data->cb;
+	skb->inner_protocol = data->inner_protocol;
+	if (data->vlan_tci & VLAN_CFI_MASK)
+		__vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci & ~VLAN_CFI_MASK);
+	else
+		__vlan_hwaccel_clear_tag(skb);
+
+	/* Reconstruct the MAC header.  */
+	skb_push(skb, data->l2_len);
+	memcpy(skb->data, &data->l2_data, data->l2_len);
+	skb_postpush_rcsum(skb, skb->data, data->l2_len);
+	skb_reset_mac_header(skb);
+
+	dev_queue_xmit(skb);
+
+	return 0;
+}
+
+static void tcf_mirred_prepare_frag(struct sk_buff *skb)
+{
+	unsigned int hlen = skb_network_offset(skb);
+	struct tcf_mirred_frag_data *data;
+
+	data = this_cpu_ptr(&tcf_mirred_frag_data_storage);
+	data->dst = skb->_skb_refdst;
+	data->cb = *qdisc_skb_cb(skb);
+	data->inner_protocol = skb->inner_protocol;
+	if (skb_vlan_tag_present(skb))
+		data->vlan_tci = skb_vlan_tag_get(skb) | VLAN_CFI_MASK;
+	else
+		data->vlan_tci = 0;
+	data->vlan_proto = skb->vlan_proto;
+	data->l2_len = hlen;
+	memcpy(&data->l2_data, skb->data, hlen);
+
+	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+	skb_pull(skb, hlen);
+}
+
+static unsigned int
+tcf_mirred_dst_get_mtu(const struct dst_entry *dst)
+{
+	return dst->dev->mtu;
+}
+
+static struct dst_ops tcf_mirred_dst_ops = {
+	.family = AF_UNSPEC,
+	.mtu = tcf_mirred_dst_get_mtu,
+};
+
+static int tcf_mirred_fragment(struct net *net, struct sk_buff *skb, u16 mru)
+{
+	if (skb_network_offset(skb) > VLAN_ETH_HLEN) {
+		net_warn_ratelimited("L2 header too long to fragment\n");
+		goto err;
+	}
+
+	if (skb->protocol == htons(ETH_P_IP)) {
+		struct dst_entry tcf_mirred_dst;
+		unsigned long orig_dst;
+
+		tcf_mirred_prepare_frag(skb);
+		dst_init(&tcf_mirred_dst, &tcf_mirred_dst_ops, NULL, 1,
+			 DST_OBSOLETE_NONE, DST_NOCOUNT);
+		tcf_mirred_dst.dev = skb->dev;
+
+		orig_dst = skb->_skb_refdst;
+		skb_dst_set_noref(skb, &tcf_mirred_dst);
+		IPCB(skb)->frag_max_size = mru;
+
+		ip_do_fragment(net, skb->sk, skb, tcf_mirred_output);
+		refdst_drop(orig_dst);
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		const struct nf_ipv6_ops *v6ops = nf_get_ipv6_ops();
+		unsigned long orig_dst;
+		struct rt6_info tcf_mirred_rt;
+
+		if (!v6ops)
+			goto err;
+
+		tcf_mirred_prepare_frag(skb);
+		memset(&tcf_mirred_rt, 0, sizeof(tcf_mirred_rt));
+		dst_init(&tcf_mirred_rt.dst, &tcf_mirred_dst_ops, NULL, 1,
+			 DST_OBSOLETE_NONE, DST_NOCOUNT);
+		tcf_mirred_rt.dst.dev = skb->dev;
+
+		orig_dst = skb->_skb_refdst;
+		skb_dst_set_noref(skb, &tcf_mirred_rt.dst);
+		IP6CB(skb)->frag_max_size = mru;
+
+		v6ops->fragment(net, skb->sk, skb, tcf_mirred_output);
+		refdst_drop(orig_dst);
+	} else {
+		net_warn_ratelimited("Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.\n",
+				     netdev_name(skb->dev), ntohs(skb->protocol),
+				     mru, skb->dev->mtu);
+		goto err;
+	}
+
+	qdisc_skb_cb(skb)->mru = 0;
+	return 0;
+err:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
+{
+	u16 mru = qdisc_skb_cb(skb)->mru;
+	int err;
+
+	if (!want_ingress)
+		if (mru && skb->len > mru + skb->dev->hard_header_len)
+			err = tcf_mirred_fragment(dev_net(skb->dev), skb, mru);
+		else
+			err = dev_queue_xmit(skb);
+	else
+		err = netif_receive_skb(skb);
+
+	return err;
+}
+
 static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 			  struct tcf_result *res)
 {
@@ -289,18 +437,15 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 		/* let's the caller reinsert the packet, if possible */
 		if (use_reinsert) {
 			res->ingress = want_ingress;
-			if (skb_tc_reinsert(skb, res))
+			err = tcf_mirred_forward(res->ingress, skb);
+			if (err)
 				tcf_action_inc_overlimit_qstats(&m->common);
 			__this_cpu_dec(mirred_rec_level);
 			return TC_ACT_CONSUMED;
 		}
 	}
 
-	if (!want_ingress)
-		err = dev_queue_xmit(skb2);
-	else
-		err = netif_receive_skb(skb2);
-
+	err = tcf_mirred_forward(want_ingress, skb2);
 	if (err) {
 out:
 		tcf_action_inc_overlimit_qstats(&m->common);
-- 
1.8.3.1

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

[Eric Dumazet]

On 6/29/20 7:54 PM, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> The fragment packets do defrag in act_ct module. The reassembled packet
> over the mtu in the act_mirred. This big packet should be fragmented
> to send out.
> 
> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
> This patch is based on
> http://patchwork.ozlabs.org/project/netdev/patch/1593422178-26949-1-git-send-email-wenxu@ucloud.cn/
> 
>  include/net/sch_generic.h |   6 +-
>  net/sched/act_ct.c        |   7 ++-
>  net/sched/act_mirred.c    | 157 ++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 158 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index c510b03..3597244 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -384,6 +384,7 @@ struct qdisc_skb_cb {
>  	};
>  #define QDISC_CB_PRIV_LEN 20
>  	unsigned char		data[QDISC_CB_PRIV_LEN];
> +	u16			mru;
>  };
> 


Wow, this change is potentially a big problem.

Explain why act_ct/act_mirred need to pollute qdisc_skb_cb 

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

[Cong Wang]

On Mon, Jun 29, 2020 at 7:55 PM <wenxu@ucloud.cn> wrote:
>
> From: wenxu <wenxu@ucloud.cn>
>
> The fragment packets do defrag in act_ct module. The reassembled packet
> over the mtu in the act_mirred. This big packet should be fragmented
> to send out.

This is too brief. Why act_mirred should handle the burden introduced by
act_ct? And why is this 158-line change targeting -net not -net-next?

Thanks.

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

[wenxu]

On 6/30/2020 11:57 PM, Eric Dumazet wrote:
>
> On 6/29/20 7:54 PM, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> The fragment packets do defrag in act_ct module. The reassembled packet
>> over the mtu in the act_mirred. This big packet should be fragmented
>> to send out.
>>
>> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>> This patch is based on
>> http://patchwork.ozlabs.org/project/netdev/patch/1593422178-26949-1-git-send-email-wenxu@ucloud.cn/
>>
>>  include/net/sch_generic.h |   6 +-
>>  net/sched/act_ct.c        |   7 ++-
>>  net/sched/act_mirred.c    | 157 ++++++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 158 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
>> index c510b03..3597244 100644
>> --- a/include/net/sch_generic.h
>> +++ b/include/net/sch_generic.h
>> @@ -384,6 +384,7 @@ struct qdisc_skb_cb {
>>  	};
>>  #define QDISC_CB_PRIV_LEN 20
>>  	unsigned char		data[QDISC_CB_PRIV_LEN];
>> +	u16			mru;
>>  };
>>
>
> Wow, this change is potentially a big problem.
>
> Explain why act_ct/act_mirred need to pollute qdisc_skb_cb 

Hi Eric,

In the act_ct the fragment packets will defrag to a big packet and do conntrack things.

But in the latter filter mirred action, the big packet normally send over the mtu of outgoing device.

In this case the packet should be fragment to send And the frag size should based on origin frag size

recored in act_ct.  In the act_ct the frag size should retore in qdisc_skb_cb and act_mirred get it to do fragment.


So there are any other good method?


BR

wenxu 

>
>
>

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

[wenxu]

On 7/1/2020 3:02 AM, Cong Wang wrote:
> On Mon, Jun 29, 2020 at 7:55 PM <wenxu@ucloud.cn> wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> The fragment packets do defrag in act_ct module. The reassembled packet
>> over the mtu in the act_mirred. This big packet should be fragmented
>> to send out.
> This is too brief. Why act_mirred should handle the burden introduced by
> act_ct? And why is this 158-line change targeting -net not -net-next?

Hi Cong,

In the act_ct the fragment packets will defrag to a big packet and do conntrack things.

But in the latter filter mirred action, the big packet normally send over the mtu of outgoing device.

So in the act_mirred send the packet should fragment. 

I think this is a bugfix in the net branch the act_ct handle with fragment will always fail.  


BR

wenxu

>
> Thanks.
>

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

[Cong Wang]

On Tue, Jun 30, 2020 at 7:36 PM wenxu <wenxu@ucloud.cn> wrote:
>
>
> On 7/1/2020 3:02 AM, Cong Wang wrote:
> > On Mon, Jun 29, 2020 at 7:55 PM <wenxu@ucloud.cn> wrote:
> >> From: wenxu <wenxu@ucloud.cn>
> >>
> >> The fragment packets do defrag in act_ct module. The reassembled packet
> >> over the mtu in the act_mirred. This big packet should be fragmented
> >> to send out.
> > This is too brief. Why act_mirred should handle the burden introduced by
> > act_ct? And why is this 158-line change targeting -net not -net-next?
>
> Hi Cong,
>
> In the act_ct the fragment packets will defrag to a big packet and do conntrack things.
>
> But in the latter filter mirred action, the big packet normally send over the mtu of outgoing device.
>
> So in the act_mirred send the packet should fragment.

Why act_mirred? Not, for a quick example, a new action called act_defrag?
I understand you happen to use the combination of act_ct and act_mirred,
but that is not the reason we should make act_mirred specifically work
for your case.

>
> I think this is a bugfix in the net branch the act_ct handle with fragment will always fail.

act_mirred is just to mirror or redirect packets, if they are too big
to deliver,
it is not act_mirred's fault.

And more importantly, why don't you put all these important information in
your changelog?

THanks.

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

[wenxu]

On 7/1/2020 1:52 PM, Cong Wang wrote:
> On Tue, Jun 30, 2020 at 7:36 PM wenxu <wenxu@ucloud.cn> wrote:
>>
>> On 7/1/2020 3:02 AM, Cong Wang wrote:
>>> On Mon, Jun 29, 2020 at 7:55 PM <wenxu@ucloud.cn> wrote:
>>>> From: wenxu <wenxu@ucloud.cn>
>>>>
>>>> The fragment packets do defrag in act_ct module. The reassembled packet
>>>> over the mtu in the act_mirred. This big packet should be fragmented
>>>> to send out.
>>> This is too brief. Why act_mirred should handle the burden introduced by
>>> act_ct? And why is this 158-line change targeting -net not -net-next?
>> Hi Cong,
>>
>> In the act_ct the fragment packets will defrag to a big packet and do conntrack things.
>>
>> But in the latter filter mirred action, the big packet normally send over the mtu of outgoing device.
>>
>> So in the act_mirred send the packet should fragment.
> Why act_mirred? Not, for a quick example, a new action called act_defrag?
> I understand you happen to use the combination of act_ct and act_mirred,
> but that is not the reason we should make act_mirred specifically work
> for your case.

Only forward packet case need do fragment again and there is no need do defrag explicit.

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

[Cong Wang]

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.

Thanks.

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

[wenxu]

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.

>
> Thanks.
>

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

[wenxu]

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?


BR

wenxu

>

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

[Cong Wang]

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.

act_mirred has the context to handle it does not mean it has to handle it.
Its name already tells you it only handles mirror or redirection, and
fragmentation is a layer 3 thing, it does not fit well in layer 2 here. This
is why you should think carefully about what is the best place to handle
it, _possibly_ it should not be in TC at all.

Thanks.

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

[wenxu]

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?


When should use the act_fragament the action,  always before the act_mirred?


rule1 in chain0

eth_type ipv4 dst_ip 1.1.1.1 ip_flags frag/firstfrag  

action order 1: ct zone 1 nat pipe

action order 2: gact action goto chain 4


rule2 in chain0

eth_type ipv4 dst_ip 1.1.1.1 ip_flags frag/nofirstfrag  

action order 1: ct zone 1 nat pipe

action order 2: gact action goto chain 4


fragment 1 and 2 do the defag and conntrack nat jump to rule3 in chain4


dst_mac fa:ff:ff:ff:ff:ff
src_mac 52:54:00:00:12:34
  eth_type ipv4
  ip_flags nofrag
  ct_state +trk+est
  ct_zone 1

action order 1: mirred (Egress Redirect to device net2) stolen


There is no definitely reason for user to set the act_fragment action. Or

there is a key  ip_frags ressembly?

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

[Marcelo Ricardo Leitner]

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.

> 
> 
> 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".

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

[Cong Wang]

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.

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

[Marcelo Ricardo Leitner]

On Thu, Jul 02, 2020 at 02:39:07PM -0700, Cong Wang wrote:
> 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.

Hmm I'm not sure you got what I meant. That's operating on the very
same skb... I meant that the pipe would handle a list of skbs (like in
netif_receive_skb_list). So when we have a goto action with such skb,
it would have to either break this list and reclassify each skb
individually, or the classification would have to do it. Either way,
it adds more complexity not just to the code but to the user as well
and ends up doing more processing (in case of fragments or not) than
if it knew how to output such packets properly. Or am I just
over-complicating it?

Or, instead of the explicit "frag" action, make act_ct re-fragment it.
It would need to, somehow, push multiple skbs down the remaining
action pipe. It boils down to the above as well.

> 
> >
> > >
> > >
> > > 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?

Not at all, but

> 
> "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.

in this case a act_output_it_well could do it. ;-)

Thanks,
  Marcelo

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

[wenxu]

On 7/3/2020 8:47 AM, Marcelo Ricardo Leitner wrote:
> On Thu, Jul 02, 2020 at 02:39:07PM -0700, Cong Wang wrote:
>> 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.
> Hmm I'm not sure you got what I meant. That's operating on the very
> same skb... I meant that the pipe would handle a list of skbs (like in
> netif_receive_skb_list). So when we have a goto action with such skb,
> it would have to either break this list and reclassify each skb
> individually, or the classification would have to do it. Either way,
> it adds more complexity not just to the code but to the user as well
> and ends up doing more processing (in case of fragments or not) than
> if it knew how to output such packets properly. Or am I just
> over-complicating it?
>
> Or, instead of the explicit "frag" action, make act_ct re-fragment it.
> It would need to, somehow, push multiple skbs down the remaining
> action pipe. It boils down to the above as well.
>
>>>>
>>>> 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?
> Not at all, but
>
>> "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.
> in this case a act_output_it_well could do it. ;-)
agree, Maybe a act_output_ct action is better?
>
> Thanks,
>   Marcelo
>

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

[Marcelo Ricardo Leitner]

On Fri, Jul 03, 2020 at 06:19:51PM +0800, wenxu wrote:
> 
> On 7/3/2020 8:47 AM, Marcelo Ricardo Leitner wrote:
> > On Thu, Jul 02, 2020 at 02:39:07PM -0700, Cong Wang wrote:
> >> 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.
> > Hmm I'm not sure you got what I meant. That's operating on the very
> > same skb... I meant that the pipe would handle a list of skbs (like in
> > netif_receive_skb_list). So when we have a goto action with such skb,
> > it would have to either break this list and reclassify each skb
> > individually, or the classification would have to do it. Either way,
> > it adds more complexity not just to the code but to the user as well
> > and ends up doing more processing (in case of fragments or not) than
> > if it knew how to output such packets properly. Or am I just
> > over-complicating it?
> >
> > Or, instead of the explicit "frag" action, make act_ct re-fragment it.
> > It would need to, somehow, push multiple skbs down the remaining
> > action pipe. It boils down to the above as well.
> >
> >>>>
> >>>> 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?
> > Not at all, but
> >
> >> "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.
> > in this case a act_output_it_well could do it. ;-)
> agree, Maybe a act_output_ct action is better?

Ahm, sorry, that wasn't really my intention here. I meant that I don't
see an issue with mirred learning how to fragment it and, with that,
do its action "well" (as subjective as the term can be).

> >
> > Thanks,
> >   Marcelo
> >

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

[wenxu]

在 2020/7/4 1:50, Marcelo Ricardo Leitner 写道:
> On Fri, Jul 03, 2020 at 06:19:51PM +0800, wenxu wrote:
>> On 7/3/2020 8:47 AM, Marcelo Ricardo Leitner wrote:
>>> On Thu, Jul 02, 2020 at 02:39:07PM -0700, Cong Wang wrote:
>>>> 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.
>>> Hmm I'm not sure you got what I meant. That's operating on the very
>>> same skb... I meant that the pipe would handle a list of skbs (like in
>>> netif_receive_skb_list). So when we have a goto action with such skb,
>>> it would have to either break this list and reclassify each skb
>>> individually, or the classification would have to do it. Either way,
>>> it adds more complexity not just to the code but to the user as well
>>> and ends up doing more processing (in case of fragments or not) than
>>> if it knew how to output such packets properly. Or am I just
>>> over-complicating it?
>>>
>>> Or, instead of the explicit "frag" action, make act_ct re-fragment it.
>>> It would need to, somehow, push multiple skbs down the remaining
>>> action pipe. It boils down to the above as well.
>>>
>>>>>> 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?
>>> Not at all, but
>>>
>>>> "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.
>>> in this case a act_output_it_well could do it. ;-)
>> agree, Maybe a act_output_ct action is better?
> Ahm, sorry, that wasn't really my intention here. I meant that I don't
> see an issue with mirred learning how to fragment it and, with that,
> do its action "well" (as subjective as the term can be).
Thanks， I also think It is ok do fragment in the mirred output.
>
>>> Thanks,
>>>   Marcelo
>>>

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

[Cong Wang]

On Thu, Jul 2, 2020 at 5:47 PM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> in this case a act_output_it_well could do it. ;-)

Yeah, still much better than making "mirred" do "mirror, redirect, frag,
defrag", can't we all agree it is too late to rename mirred? :)

Please do not try to add any arbitrary functionality into act_mirred.
Just stop here and find a better way.

Thanks.

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

[Cong Wang]

On Sun, Jul 5, 2020 at 7:33 AM wenxu <wenxu@ucloud.cn> wrote:
> Thanks， I also think It is ok do fragment in the mirred output.

Please stop doing it. There is no way to make this acceptable.

You must be smart enough to find solutions elsewhere, possibly
not even in TC at all. It would be best if you can solve this L3 problem
at L3.

Thanks!