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

[Davide Caratti <dcaratti@redhat.com>]

hello wenxu!

On Thu, 2020-11-05 at 18:41 +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> Currently kernel tc subsystem can do conntrack in act_ct. But when several
> fragment packets go through the act_ct, function tcf_ct_handle_fragments
> will defrag the packets to a big one. But the last action will redirect
> mirred to a device which maybe lead the reassembly big packet over the mtu
> of target device.
> 
> This patch add support for a xmit hook to mirred, that gets executed before
> xmiting the packet. Then, when act_ct gets loaded, it configs that hook.
> The frag xmit hook maybe reused by other modules.
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---

[...]

> +
> +static int tcf_fragment(struct net *net, struct sk_buff *skb,
> +			u16 mru, int (*xmit)(struct sk_buff *skb))
> +{
> +	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)) {

small nit: use of skb->protocol here may lead to "ambiguous" results: a
VLAN "accelerated" packet is properly processed, while the same VLAN
packet with "non-accelerated" tag is not processed because skb->protocol
is htons(ETH_P_8021Q). Can I suggest use of skb_protocol(), that has
been introduced recently by Toke [1] ?

> +		ip_do_fragment(net, skb->sk, skb, tcf_frag_xmit);
> +		refdst_drop(orig_dst);
> +	} else if (skb->protocol == htons(ETH_P_IPV6)) {

same here,

> +		unsigned long orig_dst;
> +		struct rt6_info tcf_frag_rt;
> +
> +		tcf_frag_prepare_frag(skb, xmit);
> +		memset(&tcf_frag_rt, 0, sizeof(tcf_frag_rt));
> +		dst_init(&tcf_frag_rt.dst, &tcf_frag_dst_ops, NULL, 1,
> +			 DST_OBSOLETE_NONE, DST_NOCOUNT);
> +		tcf_frag_rt.dst.dev = skb->dev;
> +
> +		orig_dst = skb->_skb_refdst;
> +		skb_dst_set_noref(skb, &tcf_frag_rt.dst);
> +		IP6CB(skb)->frag_max_size = mru;
> +
> +		ipv6_stub->ipv6_fragment(net, skb->sk, skb, tcf_frag_xmit);
> +		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);

and here (even though it's just a printout).


thanks!
-- 
davide

[1] https://lore.kernel.org/netdev/20200707110325.86731-1-toke@redhat.com/