* [PATCH v5 net-next 0/3] net/sched: fix over mtu packet of defrag in
@ 2020-11-07 23:30 wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: wenxu @ 2020-11-07 23:30 UTC (permalink / raw)
To: kuba, marcelo.leitner, dcaratti, vladbu; +Cc: netdev
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.
The first patch fix miss init the qdisc_skb_cb->mru
The send one refactor the hanle of xmit in act_mirred and prepare for the
third one
The last one add implict packet fragment support to fix the over mtu for
defrag in act_ct.
wenxu (3):
net/sched: fix miss init the mru in qdisc_skb_cb
net/sched: act_mirred: refactor the handle of xmit
net/sched: act_frag: add implict packet fragment support.
include/net/act_api.h | 16 +++++
include/net/sch_generic.h | 5 --
net/core/dev.c | 2 +
net/sched/Kconfig | 13 ++++
net/sched/Makefile | 1 +
net/sched/act_api.c | 51 ++++++++++++++
net/sched/act_ct.c | 7 ++
net/sched/act_frag.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++
net/sched/act_mirred.c | 21 ++++--
9 files changed, 269 insertions(+), 11 deletions(-)
create mode 100644 net/sched/act_frag.c
--
1.8.3.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb
2020-11-07 23:30 [PATCH v5 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
@ 2020-11-07 23:30 ` wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2 siblings, 0 replies; 10+ messages in thread
From: wenxu @ 2020-11-07 23:30 UTC (permalink / raw)
To: kuba, marcelo.leitner, dcaratti, vladbu; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
The mru in the qdisc_skb_cb should be init as 0. Only defrag packets in the
act_ct will set the value.
Fixes: 038ebb1a713d ("net/sched: act_ct: fix miss set mru for ovs after defrag in act_ct")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v5: new patch
net/core/dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 751e526..a40de66 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3865,6 +3865,7 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
return skb;
/* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
+ qdisc_skb_cb(skb)->mru = 0;
mini_qdisc_bstats_cpu_update(miniq, skb);
switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {
@@ -4950,6 +4951,7 @@ static __latent_entropy void net_tx_action(struct softirq_action *h)
}
qdisc_skb_cb(skb)->pkt_len = skb->len;
+ qdisc_skb_cb(skb)->mru = 0;
skb->tc_at_ingress = 1;
mini_qdisc_bstats_cpu_update(miniq, skb);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit
2020-11-07 23:30 [PATCH v5 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
@ 2020-11-07 23:30 ` wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2 siblings, 0 replies; 10+ messages in thread
From: wenxu @ 2020-11-07 23:30 UTC (permalink / raw)
To: kuba, marcelo.leitner, dcaratti, vladbu; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
This one is prepare for the next patch.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v5: no change
include/net/sch_generic.h | 5 -----
net/sched/act_mirred.c | 21 +++++++++++++++------
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d8fd867..dd74f06 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1281,9 +1281,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_mirred.c b/net/sched/act_mirred.c
index e24b7e2..17d0095 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -205,6 +205,18 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
return err;
}
+static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
+{
+ int err;
+
+ if (!want_ingress)
+ 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)
{
@@ -287,18 +299,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-07 23:30 [PATCH v5 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit wenxu
@ 2020-11-07 23:30 ` wenxu
2020-11-09 13:24 ` Vlad Buslov
2 siblings, 1 reply; 10+ messages in thread
From: wenxu @ 2020-11-07 23:30 UTC (permalink / raw)
To: kuba, marcelo.leitner, dcaratti, vladbu; +Cc: netdev
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>
---
v2: Fix the crash for act_frag module without load
v3: modify the kconfig describe and put tcf_xmit_hook_is_enabled
in the tcf_dev_queue_xmit, and xchg atomic for tcf_xmit_hook
v4: using skb_protocol and fix line length exceeds 80 columns
v5: no change
include/net/act_api.h | 16 +++++
net/sched/Kconfig | 13 ++++
net/sched/Makefile | 1 +
net/sched/act_api.c | 51 +++++++++++++++
net/sched/act_ct.c | 7 +++
net/sched/act_frag.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
net/sched/act_mirred.c | 2 +-
7 files changed, 253 insertions(+), 1 deletion(-)
create mode 100644 net/sched/act_frag.c
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 8721492..403a618 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -239,6 +239,22 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
struct netlink_ext_ack *newchain);
struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
struct tcf_chain *newchain);
+
+int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
+int tcf_set_xmit_hook(int (*xmit_hook)(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb)));
+void tcf_clear_xmit_hook(void);
+
+#if IS_ENABLED(CONFIG_NET_ACT_FRAG)
+int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
+#else
+static inline int tcf_frag_xmit_hook(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb))
+{
+ return 0;
+}
+#endif
+
#endif /* CONFIG_NET_CLS_ACT */
static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index a3b37d8..9a240c7 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -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.
+
config NET_ACT_CT
tristate "connection tracking tc action"
depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT && NF_FLOW_TABLE
+ depends on NET_ACT_FRAG
help
Say Y here to allow sending the packets to conntrack module.
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 66bbf9a..c146186 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_NET_IFE_SKBMARK) += act_meta_mark.o
obj-$(CONFIG_NET_IFE_SKBPRIO) += act_meta_skbprio.o
obj-$(CONFIG_NET_IFE_SKBTCINDEX) += act_meta_skbtcindex.o
obj-$(CONFIG_NET_ACT_TUNNEL_KEY)+= act_tunnel_key.o
+obj-$(CONFIG_NET_ACT_FRAG) += act_frag.o
obj-$(CONFIG_NET_ACT_CT) += act_ct.o
obj-$(CONFIG_NET_ACT_GATE) += act_gate.o
obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f66417d..e7b501c 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -22,6 +22,57 @@
#include <net/act_api.h>
#include <net/netlink.h>
+static int (*tcf_xmit_hook)(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb));
+static DEFINE_STATIC_KEY_FALSE(tcf_xmit_hook_in_use);
+
+static void tcf_inc_xmit_hook(void)
+{
+ static_branch_inc(&tcf_xmit_hook_in_use);
+}
+
+static void tcf_dec_xmit_hook(void)
+{
+ static_branch_dec(&tcf_xmit_hook_in_use);
+}
+
+static bool tcf_xmit_hook_enabled(void)
+{
+ return static_branch_unlikely(&tcf_xmit_hook_in_use);
+}
+
+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);
+ 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())
+ return tcf_xmit_hook(skb, xmit);
+ else
+ return xmit(skb);
+}
+EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
+
static void tcf_action_goto_chain_exec(const struct tc_action *a,
struct tcf_result *res)
{
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 9c79fb9..dff3c40 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
if (err)
goto err_register;
+ err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
+ if (err)
+ goto err_action;
+
return 0;
+err_action:
+ tcf_unregister_action(&act_ct_ops, &ct_net_ops);
err_register:
tcf_ct_flow_tables_uninit();
err_tbl_init:
@@ -1552,6 +1558,7 @@ static int __init ct_init_module(void)
static void __exit ct_cleanup_module(void)
{
+ tcf_clear_xmit_hook();
tcf_unregister_action(&act_ct_ops, &ct_net_ops);
tcf_ct_flow_tables_uninit();
destroy_workqueue(act_ct_wq);
diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
new file mode 100644
index 0000000..3a7ab92
--- /dev/null
+++ b/net/sched/act_frag.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+#include <net/netlink.h>
+#include <net/act_api.h>
+#include <net/dst.h>
+#include <net/ip.h>
+#include <net/ip6_fib.h>
+
+struct tcf_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];
+ int (*xmit)(struct sk_buff *skb);
+};
+
+static DEFINE_PER_CPU(struct tcf_frag_data, tcf_frag_data_storage);
+
+static int tcf_frag_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
+{
+ struct tcf_frag_data *data = this_cpu_ptr(&tcf_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);
+
+ data->xmit(skb);
+
+ return 0;
+}
+
+static void tcf_frag_prepare_frag(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb))
+{
+ unsigned int hlen = skb_network_offset(skb);
+ struct tcf_frag_data *data;
+
+ data = this_cpu_ptr(&tcf_frag_data_storage);
+ data->dst = skb->_skb_refdst;
+ data->cb = *qdisc_skb_cb(skb);
+ data->xmit = xmit;
+ 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_frag_dst_get_mtu(const struct dst_entry *dst)
+{
+ return dst->dev->mtu;
+}
+
+static struct dst_ops tcf_frag_dst_ops = {
+ .family = AF_UNSPEC,
+ .mtu = tcf_frag_dst_get_mtu,
+};
+
+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(skb, true) == htons(ETH_P_IP)) {
+ struct dst_entry tcf_frag_dst;
+ unsigned long orig_dst;
+
+ tcf_frag_prepare_frag(skb, xmit);
+ dst_init(&tcf_frag_dst, &tcf_frag_dst_ops, NULL, 1,
+ DST_OBSOLETE_NONE, DST_NOCOUNT);
+ tcf_frag_dst.dev = skb->dev;
+
+ orig_dst = skb->_skb_refdst;
+ skb_dst_set_noref(skb, &tcf_frag_dst);
+ IPCB(skb)->frag_max_size = mru;
+
+ ip_do_fragment(net, skb->sk, skb, tcf_frag_xmit);
+ refdst_drop(orig_dst);
+ } else if (skb_protocol(skb, true) == htons(ETH_P_IPV6)) {
+ 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("Fail frag %s: eth=%x, MRU=%d, MTU=%d\n",
+ netdev_name(skb->dev),
+ ntohs(skb_protocol(skb, true)), mru,
+ skb->dev->mtu);
+ goto err;
+ }
+
+ qdisc_skb_cb(skb)->mru = 0;
+ return 0;
+err:
+ kfree_skb(skb);
+ return -1;
+}
+
+int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
+{
+ u16 mru = qdisc_skb_cb(skb)->mru;
+ int err;
+
+ if (mru && skb->len > mru + skb->dev->hard_header_len)
+ err = tcf_fragment(dev_net(skb->dev), skb, mru, xmit);
+ else
+ err = xmit(skb);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(tcf_frag_xmit_hook);
+
+static int __init frag_init_module(void)
+{
+ return 0;
+}
+
+static void __exit frag_cleanup_module(void)
+{
+}
+
+module_init(frag_init_module);
+module_exit(frag_cleanup_module);
+MODULE_AUTHOR("wenxu <wenxu@ucloud.cn>");
+MODULE_LICENSE("GPL v2");
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 17d0095..7153c67 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -210,7 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
int err;
if (!want_ingress)
- err = dev_queue_xmit(skb);
+ err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
else
err = netif_receive_skb(skb);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-07 23:30 ` [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
@ 2020-11-09 13:24 ` Vlad Buslov
2020-11-09 14:50 ` Marcelo Ricardo Leitner
2020-11-09 14:54 ` wenxu
0 siblings, 2 replies; 10+ messages in thread
From: Vlad Buslov @ 2020-11-09 13:24 UTC (permalink / raw)
To: marcelo.leitner, wenxu; +Cc: kuba, dcaratti, netdev
On Sun 08 Nov 2020 at 01:30, 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>
> ---
> v2: Fix the crash for act_frag module without load
> v3: modify the kconfig describe and put tcf_xmit_hook_is_enabled
> in the tcf_dev_queue_xmit, and xchg atomic for tcf_xmit_hook
> v4: using skb_protocol and fix line length exceeds 80 columns
> v5: no change
>
> include/net/act_api.h | 16 +++++
> net/sched/Kconfig | 13 ++++
> net/sched/Makefile | 1 +
> net/sched/act_api.c | 51 +++++++++++++++
> net/sched/act_ct.c | 7 +++
> net/sched/act_frag.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/sched/act_mirred.c | 2 +-
> 7 files changed, 253 insertions(+), 1 deletion(-)
> create mode 100644 net/sched/act_frag.c
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 8721492..403a618 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -239,6 +239,22 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> struct netlink_ext_ack *newchain);
> struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
> struct tcf_chain *newchain);
> +
> +int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
> +int tcf_set_xmit_hook(int (*xmit_hook)(struct sk_buff *skb,
> + int (*xmit)(struct sk_buff *skb)));
> +void tcf_clear_xmit_hook(void);
> +
> +#if IS_ENABLED(CONFIG_NET_ACT_FRAG)
> +int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
> +#else
> +static inline int tcf_frag_xmit_hook(struct sk_buff *skb,
> + int (*xmit)(struct sk_buff *skb))
> +{
> + return 0;
> +}
> +#endif
> +
> #endif /* CONFIG_NET_CLS_ACT */
>
> static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index a3b37d8..9a240c7 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -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?
> config NET_ACT_CT
> tristate "connection tracking tc action"
> depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT && NF_FLOW_TABLE
> + depends on NET_ACT_FRAG
> help
> Say Y here to allow sending the packets to conntrack module.
>
> diff --git a/net/sched/Makefile b/net/sched/Makefile
> index 66bbf9a..c146186 100644
> --- a/net/sched/Makefile
> +++ b/net/sched/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_NET_IFE_SKBMARK) += act_meta_mark.o
> obj-$(CONFIG_NET_IFE_SKBPRIO) += act_meta_skbprio.o
> obj-$(CONFIG_NET_IFE_SKBTCINDEX) += act_meta_skbtcindex.o
> obj-$(CONFIG_NET_ACT_TUNNEL_KEY)+= act_tunnel_key.o
> +obj-$(CONFIG_NET_ACT_FRAG) += act_frag.o
> obj-$(CONFIG_NET_ACT_CT) += act_ct.o
> obj-$(CONFIG_NET_ACT_GATE) += act_gate.o
> obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index f66417d..e7b501c 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -22,6 +22,57 @@
> #include <net/act_api.h>
> #include <net/netlink.h>
>
> +static int (*tcf_xmit_hook)(struct sk_buff *skb,
> + int (*xmit)(struct sk_buff *skb));
> +static DEFINE_STATIC_KEY_FALSE(tcf_xmit_hook_in_use);
> +
> +static void tcf_inc_xmit_hook(void)
> +{
> + static_branch_inc(&tcf_xmit_hook_in_use);
> +}
> +
> +static void tcf_dec_xmit_hook(void)
> +{
> + static_branch_dec(&tcf_xmit_hook_in_use);
> +}
> +
> +static bool tcf_xmit_hook_enabled(void)
> +{
> + return static_branch_unlikely(&tcf_xmit_hook_in_use);
> +}
> +
> +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.
> + 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?
> + return tcf_xmit_hook(skb, xmit);
> + else
> + return xmit(skb);
> +}
> +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
> +
> static void tcf_action_goto_chain_exec(const struct tc_action *a,
> struct tcf_result *res)
> {
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 9c79fb9..dff3c40 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
> if (err)
> goto err_register;
>
> + err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
> + if (err)
> + goto err_action;
> +
> return 0;
>
> +err_action:
> + tcf_unregister_action(&act_ct_ops, &ct_net_ops);
> err_register:
> tcf_ct_flow_tables_uninit();
> err_tbl_init:
> @@ -1552,6 +1558,7 @@ static int __init ct_init_module(void)
>
> static void __exit ct_cleanup_module(void)
> {
> + tcf_clear_xmit_hook();
> tcf_unregister_action(&act_ct_ops, &ct_net_ops);
> tcf_ct_flow_tables_uninit();
> destroy_workqueue(act_ct_wq);
> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
> new file mode 100644
> index 0000000..3a7ab92
> --- /dev/null
> +++ b/net/sched/act_frag.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +#include <net/netlink.h>
> +#include <net/act_api.h>
> +#include <net/dst.h>
> +#include <net/ip.h>
> +#include <net/ip6_fib.h>
> +
> +struct tcf_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];
> + int (*xmit)(struct sk_buff *skb);
> +};
> +
> +static DEFINE_PER_CPU(struct tcf_frag_data, tcf_frag_data_storage);
> +
> +static int tcf_frag_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
> +{
> + struct tcf_frag_data *data = this_cpu_ptr(&tcf_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);
> +
> + data->xmit(skb);
> +
> + return 0;
> +}
> +
> +static void tcf_frag_prepare_frag(struct sk_buff *skb,
> + int (*xmit)(struct sk_buff *skb))
> +{
> + unsigned int hlen = skb_network_offset(skb);
> + struct tcf_frag_data *data;
> +
> + data = this_cpu_ptr(&tcf_frag_data_storage);
> + data->dst = skb->_skb_refdst;
> + data->cb = *qdisc_skb_cb(skb);
> + data->xmit = xmit;
> + 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_frag_dst_get_mtu(const struct dst_entry *dst)
> +{
> + return dst->dev->mtu;
> +}
> +
> +static struct dst_ops tcf_frag_dst_ops = {
> + .family = AF_UNSPEC,
> + .mtu = tcf_frag_dst_get_mtu,
> +};
> +
> +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(skb, true) == htons(ETH_P_IP)) {
> + struct dst_entry tcf_frag_dst;
> + unsigned long orig_dst;
> +
> + tcf_frag_prepare_frag(skb, xmit);
> + dst_init(&tcf_frag_dst, &tcf_frag_dst_ops, NULL, 1,
> + DST_OBSOLETE_NONE, DST_NOCOUNT);
> + tcf_frag_dst.dev = skb->dev;
> +
> + orig_dst = skb->_skb_refdst;
> + skb_dst_set_noref(skb, &tcf_frag_dst);
> + IPCB(skb)->frag_max_size = mru;
> +
> + ip_do_fragment(net, skb->sk, skb, tcf_frag_xmit);
> + refdst_drop(orig_dst);
> + } else if (skb_protocol(skb, true) == htons(ETH_P_IPV6)) {
> + 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("Fail frag %s: eth=%x, MRU=%d, MTU=%d\n",
> + netdev_name(skb->dev),
> + ntohs(skb_protocol(skb, true)), mru,
> + skb->dev->mtu);
> + goto err;
> + }
> +
> + qdisc_skb_cb(skb)->mru = 0;
> + return 0;
> +err:
> + kfree_skb(skb);
> + return -1;
> +}
> +
> +int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
> +{
> + u16 mru = qdisc_skb_cb(skb)->mru;
> + int err;
> +
> + if (mru && skb->len > mru + skb->dev->hard_header_len)
> + err = tcf_fragment(dev_net(skb->dev), skb, mru, xmit);
> + else
> + err = xmit(skb);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(tcf_frag_xmit_hook);
> +
> +static int __init frag_init_module(void)
> +{
> + return 0;
> +}
> +
> +static void __exit frag_cleanup_module(void)
> +{
> +}
> +
> +module_init(frag_init_module);
> +module_exit(frag_cleanup_module);
> +MODULE_AUTHOR("wenxu <wenxu@ucloud.cn>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 17d0095..7153c67 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -210,7 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
> int err;
>
> if (!want_ingress)
> - err = dev_queue_xmit(skb);
> + err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
> else
> err = netif_receive_skb(skb);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-09 13:24 ` Vlad Buslov
@ 2020-11-09 14:50 ` Marcelo Ricardo Leitner
2020-11-09 15:47 ` Vlad Buslov
2020-11-09 14:54 ` wenxu
1 sibling, 1 reply; 10+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-09 14:50 UTC (permalink / raw)
To: Vlad Buslov; +Cc: wenxu, kuba, dcaratti, netdev
On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
> On Sun 08 Nov 2020 at 01:30, wenxu@ucloud.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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-09 13:24 ` Vlad Buslov
2020-11-09 14:50 ` Marcelo Ricardo Leitner
@ 2020-11-09 14:54 ` wenxu
2020-11-09 15:51 ` Vlad Buslov
1 sibling, 1 reply; 10+ messages in thread
From: wenxu @ 2020-11-09 14:54 UTC (permalink / raw)
To: Vlad Buslov, marcelo.leitner; +Cc: kuba, dcaratti, netdev
在 2020/11/9 21:24, Vlad Buslov 写道:
> On Sun 08 Nov 2020 at 01:30, 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>
>> ---
>> v2: Fix the crash for act_frag module without load
>> v3: modify the kconfig describe and put tcf_xmit_hook_is_enabled
>> in the tcf_dev_queue_xmit, and xchg atomic for tcf_xmit_hook
>> v4: using skb_protocol and fix line length exceeds 80 columns
>> v5: no change
>>
>> include/net/act_api.h | 16 +++++
>> net/sched/Kconfig | 13 ++++
>> net/sched/Makefile | 1 +
>> net/sched/act_api.c | 51 +++++++++++++++
>> net/sched/act_ct.c | 7 +++
>> net/sched/act_frag.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
>> net/sched/act_mirred.c | 2 +-
>> 7 files changed, 253 insertions(+), 1 deletion(-)
>> create mode 100644 net/sched/act_frag.c
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index 8721492..403a618 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -239,6 +239,22 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>> struct netlink_ext_ack *newchain);
>> struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
>> struct tcf_chain *newchain);
>> +
>> +int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
>> +int tcf_set_xmit_hook(int (*xmit_hook)(struct sk_buff *skb,
>> + int (*xmit)(struct sk_buff *skb)));
>> +void tcf_clear_xmit_hook(void);
>> +
>> +#if IS_ENABLED(CONFIG_NET_ACT_FRAG)
>> +int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
>> +#else
>> +static inline int tcf_frag_xmit_hook(struct sk_buff *skb,
>> + int (*xmit)(struct sk_buff *skb))
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> #endif /* CONFIG_NET_CLS_ACT */
>>
>> static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>> index a3b37d8..9a240c7 100644
>> --- a/net/sched/Kconfig
>> +++ b/net/sched/Kconfig
>> @@ -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?
The fragment operation is an single L3 action.
So we put in an single modules. Maybe it is not proper to put in the act_api directly.
>> config NET_ACT_CT
>> tristate "connection tracking tc action"
>> depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT && NF_FLOW_TABLE
>> + depends on NET_ACT_FRAG
>> help
>> Say Y here to allow sending the packets to conntrack module.
>>
>> diff --git a/net/sched/Makefile b/net/sched/Makefile
>> index 66bbf9a..c146186 100644
>> --- a/net/sched/Makefile
>> +++ b/net/sched/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_NET_IFE_SKBMARK) += act_meta_mark.o
>> obj-$(CONFIG_NET_IFE_SKBPRIO) += act_meta_skbprio.o
>> obj-$(CONFIG_NET_IFE_SKBTCINDEX) += act_meta_skbtcindex.o
>> obj-$(CONFIG_NET_ACT_TUNNEL_KEY)+= act_tunnel_key.o
>> +obj-$(CONFIG_NET_ACT_FRAG) += act_frag.o
>> obj-$(CONFIG_NET_ACT_CT) += act_ct.o
>> obj-$(CONFIG_NET_ACT_GATE) += act_gate.o
>> obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index f66417d..e7b501c 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -22,6 +22,57 @@
>> #include <net/act_api.h>
>> #include <net/netlink.h>
>>
>> +static int (*tcf_xmit_hook)(struct sk_buff *skb,
>> + int (*xmit)(struct sk_buff *skb));
>> +static DEFINE_STATIC_KEY_FALSE(tcf_xmit_hook_in_use);
>> +
>> +static void tcf_inc_xmit_hook(void)
>> +{
>> + static_branch_inc(&tcf_xmit_hook_in_use);
>> +}
>> +
>> +static void tcf_dec_xmit_hook(void)
>> +{
>> + static_branch_dec(&tcf_xmit_hook_in_use);
>> +}
>> +
>> +static bool tcf_xmit_hook_enabled(void)
>> +{
>> + return static_branch_unlikely(&tcf_xmit_hook_in_use);
>> +}
>> +
>> +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.
>
>> + 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?
Yes. Maybe the tcf_xmit_hook is protected by a rcu lock is better?
>
>> + return tcf_xmit_hook(skb, xmit);
>> + else
>> + return xmit(skb);
>> +}
>> +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
>> +
>> static void tcf_action_goto_chain_exec(const struct tc_action *a,
>> struct tcf_result *res)
>> {
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index 9c79fb9..dff3c40 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
>> if (err)
>> goto err_register;
>>
>> + err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
>> + if (err)
>> + goto err_action;
>> +
>> return 0;
>>
>> +err_action:
>> + tcf_unregister_action(&act_ct_ops, &ct_net_ops);
>> err_register:
>> tcf_ct_flow_tables_uninit();
>> err_tbl_init:
>> @@ -1552,6 +1558,7 @@ static int __init ct_init_module(void)
>>
>> static void __exit ct_cleanup_module(void)
>> {
>> + tcf_clear_xmit_hook();
>> tcf_unregister_action(&act_ct_ops, &ct_net_ops);
>> tcf_ct_flow_tables_uninit();
>> destroy_workqueue(act_ct_wq);
>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>> new file mode 100644
>> index 0000000..3a7ab92
>> --- /dev/null
>> +++ b/net/sched/act_frag.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> +#include <net/netlink.h>
>> +#include <net/act_api.h>
>> +#include <net/dst.h>
>> +#include <net/ip.h>
>> +#include <net/ip6_fib.h>
>> +
>> +struct tcf_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];
>> + int (*xmit)(struct sk_buff *skb);
>> +};
>> +
>> +static DEFINE_PER_CPU(struct tcf_frag_data, tcf_frag_data_storage);
>> +
>> +static int tcf_frag_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
>> +{
>> + struct tcf_frag_data *data = this_cpu_ptr(&tcf_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);
>> +
>> + data->xmit(skb);
>> +
>> + return 0;
>> +}
>> +
>> +static void tcf_frag_prepare_frag(struct sk_buff *skb,
>> + int (*xmit)(struct sk_buff *skb))
>> +{
>> + unsigned int hlen = skb_network_offset(skb);
>> + struct tcf_frag_data *data;
>> +
>> + data = this_cpu_ptr(&tcf_frag_data_storage);
>> + data->dst = skb->_skb_refdst;
>> + data->cb = *qdisc_skb_cb(skb);
>> + data->xmit = xmit;
>> + 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_frag_dst_get_mtu(const struct dst_entry *dst)
>> +{
>> + return dst->dev->mtu;
>> +}
>> +
>> +static struct dst_ops tcf_frag_dst_ops = {
>> + .family = AF_UNSPEC,
>> + .mtu = tcf_frag_dst_get_mtu,
>> +};
>> +
>> +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(skb, true) == htons(ETH_P_IP)) {
>> + struct dst_entry tcf_frag_dst;
>> + unsigned long orig_dst;
>> +
>> + tcf_frag_prepare_frag(skb, xmit);
>> + dst_init(&tcf_frag_dst, &tcf_frag_dst_ops, NULL, 1,
>> + DST_OBSOLETE_NONE, DST_NOCOUNT);
>> + tcf_frag_dst.dev = skb->dev;
>> +
>> + orig_dst = skb->_skb_refdst;
>> + skb_dst_set_noref(skb, &tcf_frag_dst);
>> + IPCB(skb)->frag_max_size = mru;
>> +
>> + ip_do_fragment(net, skb->sk, skb, tcf_frag_xmit);
>> + refdst_drop(orig_dst);
>> + } else if (skb_protocol(skb, true) == htons(ETH_P_IPV6)) {
>> + 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("Fail frag %s: eth=%x, MRU=%d, MTU=%d\n",
>> + netdev_name(skb->dev),
>> + ntohs(skb_protocol(skb, true)), mru,
>> + skb->dev->mtu);
>> + goto err;
>> + }
>> +
>> + qdisc_skb_cb(skb)->mru = 0;
>> + return 0;
>> +err:
>> + kfree_skb(skb);
>> + return -1;
>> +}
>> +
>> +int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
>> +{
>> + u16 mru = qdisc_skb_cb(skb)->mru;
>> + int err;
>> +
>> + if (mru && skb->len > mru + skb->dev->hard_header_len)
>> + err = tcf_fragment(dev_net(skb->dev), skb, mru, xmit);
>> + else
>> + err = xmit(skb);
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(tcf_frag_xmit_hook);
>> +
>> +static int __init frag_init_module(void)
>> +{
>> + return 0;
>> +}
>> +
>> +static void __exit frag_cleanup_module(void)
>> +{
>> +}
>> +
>> +module_init(frag_init_module);
>> +module_exit(frag_cleanup_module);
>> +MODULE_AUTHOR("wenxu <wenxu@ucloud.cn>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>> index 17d0095..7153c67 100644
>> --- a/net/sched/act_mirred.c
>> +++ b/net/sched/act_mirred.c
>> @@ -210,7 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
>> int err;
>>
>> if (!want_ingress)
>> - err = dev_queue_xmit(skb);
>> + err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
>> else
>> err = netif_receive_skb(skb);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-09 14:50 ` Marcelo Ricardo Leitner
@ 2020-11-09 15:47 ` Vlad Buslov
2020-11-09 16:44 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 10+ messages in thread
From: Vlad Buslov @ 2020-11-09 15:47 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: wenxu, kuba, dcaratti, netdev
On Mon 09 Nov 2020 at 16:50, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
>> On Sun 08 Nov 2020 at 01:30, wenxu@ucloud.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.
Hmmm okay.
>
> If act_frag is confusing, then maybe act_mirred_frag? It is a mirred
> plugin now, after all.
Would be even more confusing to me since the act_frag module code is
only directly accessed from act_ct and not act_mirred :)
Anyway, I don't have a strong opinion regarding this. Just wanted to
understand the motivation.
>
> ...
>> > +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.
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.
>
> 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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-09 14:54 ` wenxu
@ 2020-11-09 15:51 ` Vlad Buslov
0 siblings, 0 replies; 10+ messages in thread
From: Vlad Buslov @ 2020-11-09 15:51 UTC (permalink / raw)
To: wenxu; +Cc: marcelo.leitner, kuba, dcaratti, netdev
On Mon 09 Nov 2020 at 16:54, wenxu <wenxu@ucloud.cn> wrote:
> 在 2020/11/9 21:24, Vlad Buslov 写道:
>> On Sun 08 Nov 2020 at 01:30, 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>
>>> ---
>>> v2: Fix the crash for act_frag module without load
>>> v3: modify the kconfig describe and put tcf_xmit_hook_is_enabled
>>> in the tcf_dev_queue_xmit, and xchg atomic for tcf_xmit_hook
>>> v4: using skb_protocol and fix line length exceeds 80 columns
>>> v5: no change
>>>
>>> include/net/act_api.h | 16 +++++
>>> net/sched/Kconfig | 13 ++++
>>> net/sched/Makefile | 1 +
>>> net/sched/act_api.c | 51 +++++++++++++++
>>> net/sched/act_ct.c | 7 +++
>>> net/sched/act_frag.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> net/sched/act_mirred.c | 2 +-
>>> 7 files changed, 253 insertions(+), 1 deletion(-)
>>> create mode 100644 net/sched/act_frag.c
>>>
>>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>>> index 8721492..403a618 100644
>>> --- a/include/net/act_api.h
>>> +++ b/include/net/act_api.h
>>> @@ -239,6 +239,22 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>>> struct netlink_ext_ack *newchain);
>>> struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
>>> struct tcf_chain *newchain);
>>> +
>>> +int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
>>> +int tcf_set_xmit_hook(int (*xmit_hook)(struct sk_buff *skb,
>>> + int (*xmit)(struct sk_buff *skb)));
>>> +void tcf_clear_xmit_hook(void);
>>> +
>>> +#if IS_ENABLED(CONFIG_NET_ACT_FRAG)
>>> +int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
>>> +#else
>>> +static inline int tcf_frag_xmit_hook(struct sk_buff *skb,
>>> + int (*xmit)(struct sk_buff *skb))
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> #endif /* CONFIG_NET_CLS_ACT */
>>>
>>> static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
>>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>>> index a3b37d8..9a240c7 100644
>>> --- a/net/sched/Kconfig
>>> +++ b/net/sched/Kconfig
>>> @@ -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?
>
> The fragment operation is an single L3 action.
>
> So we put in an single modules. Maybe it is not proper to put in the act_api directly.
Okay. I have no strong opinion regarding this so it is better for
maintainers to decide.
>
>>> config NET_ACT_CT
>>> tristate "connection tracking tc action"
>>> depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT && NF_FLOW_TABLE
>>> + depends on NET_ACT_FRAG
>>> help
>>> Say Y here to allow sending the packets to conntrack module.
>>>
>>> diff --git a/net/sched/Makefile b/net/sched/Makefile
>>> index 66bbf9a..c146186 100644
>>> --- a/net/sched/Makefile
>>> +++ b/net/sched/Makefile
>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_NET_IFE_SKBMARK) += act_meta_mark.o
>>> obj-$(CONFIG_NET_IFE_SKBPRIO) += act_meta_skbprio.o
>>> obj-$(CONFIG_NET_IFE_SKBTCINDEX) += act_meta_skbtcindex.o
>>> obj-$(CONFIG_NET_ACT_TUNNEL_KEY)+= act_tunnel_key.o
>>> +obj-$(CONFIG_NET_ACT_FRAG) += act_frag.o
>>> obj-$(CONFIG_NET_ACT_CT) += act_ct.o
>>> obj-$(CONFIG_NET_ACT_GATE) += act_gate.o
>>> obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
>>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>> index f66417d..e7b501c 100644
>>> --- a/net/sched/act_api.c
>>> +++ b/net/sched/act_api.c
>>> @@ -22,6 +22,57 @@
>>> #include <net/act_api.h>
>>> #include <net/netlink.h>
>>>
>>> +static int (*tcf_xmit_hook)(struct sk_buff *skb,
>>> + int (*xmit)(struct sk_buff *skb));
>>> +static DEFINE_STATIC_KEY_FALSE(tcf_xmit_hook_in_use);
>>> +
>>> +static void tcf_inc_xmit_hook(void)
>>> +{
>>> + static_branch_inc(&tcf_xmit_hook_in_use);
>>> +}
>>> +
>>> +static void tcf_dec_xmit_hook(void)
>>> +{
>>> + static_branch_dec(&tcf_xmit_hook_in_use);
>>> +}
>>> +
>>> +static bool tcf_xmit_hook_enabled(void)
>>> +{
>>> + return static_branch_unlikely(&tcf_xmit_hook_in_use);
>>> +}
>>> +
>>> +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.
>>
>>> + 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?
>
>
> Yes. Maybe the tcf_xmit_hook is protected by a rcu lock is better?
Agree. See my response to Marcelo.
>
>>
>>> + return tcf_xmit_hook(skb, xmit);
>>> + else
>>> + return xmit(skb);
>>> +}
>>> +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
>>> +
>>> static void tcf_action_goto_chain_exec(const struct tc_action *a,
>>> struct tcf_result *res)
>>> {
>>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>>> index 9c79fb9..dff3c40 100644
>>> --- a/net/sched/act_ct.c
>>> +++ b/net/sched/act_ct.c
>>> @@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
>>> if (err)
>>> goto err_register;
>>>
>>> + err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
>>> + if (err)
>>> + goto err_action;
>>> +
>>> return 0;
>>>
>>> +err_action:
>>> + tcf_unregister_action(&act_ct_ops, &ct_net_ops);
>>> err_register:
>>> tcf_ct_flow_tables_uninit();
>>> err_tbl_init:
>>> @@ -1552,6 +1558,7 @@ static int __init ct_init_module(void)
>>>
>>> static void __exit ct_cleanup_module(void)
>>> {
>>> + tcf_clear_xmit_hook();
>>> tcf_unregister_action(&act_ct_ops, &ct_net_ops);
>>> tcf_ct_flow_tables_uninit();
>>> destroy_workqueue(act_ct_wq);
>>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>>> new file mode 100644
>>> index 0000000..3a7ab92
>>> --- /dev/null
>>> +++ b/net/sched/act_frag.c
>>> @@ -0,0 +1,164 @@
>>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>> +#include <net/netlink.h>
>>> +#include <net/act_api.h>
>>> +#include <net/dst.h>
>>> +#include <net/ip.h>
>>> +#include <net/ip6_fib.h>
>>> +
>>> +struct tcf_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];
>>> + int (*xmit)(struct sk_buff *skb);
>>> +};
>>> +
>>> +static DEFINE_PER_CPU(struct tcf_frag_data, tcf_frag_data_storage);
>>> +
>>> +static int tcf_frag_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
>>> +{
>>> + struct tcf_frag_data *data = this_cpu_ptr(&tcf_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);
>>> +
>>> + data->xmit(skb);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void tcf_frag_prepare_frag(struct sk_buff *skb,
>>> + int (*xmit)(struct sk_buff *skb))
>>> +{
>>> + unsigned int hlen = skb_network_offset(skb);
>>> + struct tcf_frag_data *data;
>>> +
>>> + data = this_cpu_ptr(&tcf_frag_data_storage);
>>> + data->dst = skb->_skb_refdst;
>>> + data->cb = *qdisc_skb_cb(skb);
>>> + data->xmit = xmit;
>>> + 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_frag_dst_get_mtu(const struct dst_entry *dst)
>>> +{
>>> + return dst->dev->mtu;
>>> +}
>>> +
>>> +static struct dst_ops tcf_frag_dst_ops = {
>>> + .family = AF_UNSPEC,
>>> + .mtu = tcf_frag_dst_get_mtu,
>>> +};
>>> +
>>> +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(skb, true) == htons(ETH_P_IP)) {
>>> + struct dst_entry tcf_frag_dst;
>>> + unsigned long orig_dst;
>>> +
>>> + tcf_frag_prepare_frag(skb, xmit);
>>> + dst_init(&tcf_frag_dst, &tcf_frag_dst_ops, NULL, 1,
>>> + DST_OBSOLETE_NONE, DST_NOCOUNT);
>>> + tcf_frag_dst.dev = skb->dev;
>>> +
>>> + orig_dst = skb->_skb_refdst;
>>> + skb_dst_set_noref(skb, &tcf_frag_dst);
>>> + IPCB(skb)->frag_max_size = mru;
>>> +
>>> + ip_do_fragment(net, skb->sk, skb, tcf_frag_xmit);
>>> + refdst_drop(orig_dst);
>>> + } else if (skb_protocol(skb, true) == htons(ETH_P_IPV6)) {
>>> + 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("Fail frag %s: eth=%x, MRU=%d, MTU=%d\n",
>>> + netdev_name(skb->dev),
>>> + ntohs(skb_protocol(skb, true)), mru,
>>> + skb->dev->mtu);
>>> + goto err;
>>> + }
>>> +
>>> + qdisc_skb_cb(skb)->mru = 0;
>>> + return 0;
>>> +err:
>>> + kfree_skb(skb);
>>> + return -1;
>>> +}
>>> +
>>> +int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
>>> +{
>>> + u16 mru = qdisc_skb_cb(skb)->mru;
>>> + int err;
>>> +
>>> + if (mru && skb->len > mru + skb->dev->hard_header_len)
>>> + err = tcf_fragment(dev_net(skb->dev), skb, mru, xmit);
>>> + else
>>> + err = xmit(skb);
>>> +
>>> + return err;
>>> +}
>>> +EXPORT_SYMBOL_GPL(tcf_frag_xmit_hook);
>>> +
>>> +static int __init frag_init_module(void)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static void __exit frag_cleanup_module(void)
>>> +{
>>> +}
>>> +
>>> +module_init(frag_init_module);
>>> +module_exit(frag_cleanup_module);
>>> +MODULE_AUTHOR("wenxu <wenxu@ucloud.cn>");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>>> index 17d0095..7153c67 100644
>>> --- a/net/sched/act_mirred.c
>>> +++ b/net/sched/act_mirred.c
>>> @@ -210,7 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
>>> int err;
>>>
>>> if (!want_ingress)
>>> - err = dev_queue_xmit(skb);
>>> + err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
>>> else
>>> err = netif_receive_skb(skb);
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-09 15:47 ` Vlad Buslov
@ 2020-11-09 16:44 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 10+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-09 16:44 UTC (permalink / raw)
To: Vlad Buslov; +Cc: wenxu, kuba, dcaratti, netdev
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.leitner@gmail.com> wrote:
> > On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
> >> On Sun 08 Nov 2020 at 01:30, wenxu@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);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-09 16:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-07 23:30 [PATCH v5 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit wenxu
2020-11-07 23:30 ` [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2020-11-09 13:24 ` Vlad Buslov
2020-11-09 14:50 ` Marcelo Ricardo Leitner
2020-11-09 15:47 ` Vlad Buslov
2020-11-09 16:44 ` Marcelo Ricardo Leitner
2020-11-09 14:54 ` wenxu
2020-11-09 15:51 ` Vlad Buslov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).