Message ID | 1595072073-6268-1-git-send-email-wenxu@ucloud.cn |
---|---|
State | Superseded |
Delegated to: | David Miller |
Headers | show |
Series | [net] net/sched: act_ct: fix restore the qdisc_skb_cb after defrag | expand |
wenxu@ucloud.cn <wenxu@ucloud.cn> wrote: > From: wenxu <wenxu@ucloud.cn> > > The fragment packets do defrag in tcf_ct_handle_fragments > will clear the skb->cb which make the qdisc_skb_cb clear > too. So the qdsic_skb_cb should be store before defrag and > restore after that. > It also update the pkt_len after all the > fragments finish the defrag to one packet and make the > following actions counter correct. > > Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct") > Signed-off-by: wenxu <wenxu@ucloud.cn> Looks ok to me. One question: > @@ -1014,6 +1017,7 @@ 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; > return retval; This appears to be unconditional, I would have expected that this only done for reassembled skbs? Otherwise we will lose the value calculated by core via qdisc_calculate_pkt_len().
在 2020/7/18 20:30, Florian Westphal 写道: > wenxu@ucloud.cn <wenxu@ucloud.cn> wrote: >> From: wenxu <wenxu@ucloud.cn> >> >> The fragment packets do defrag in tcf_ct_handle_fragments >> will clear the skb->cb which make the qdisc_skb_cb clear >> too. So the qdsic_skb_cb should be store before defrag and >> restore after that. >> It also update the pkt_len after all the >> fragments finish the defrag to one packet and make the >> following actions counter correct. >> >> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct") >> Signed-off-by: wenxu <wenxu@ucloud.cn> > Looks ok to me. One question: > >> @@ -1014,6 +1017,7 @@ 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; >> return retval; > This appears to be unconditional, I would have expected that > this only done for reassembled skbs? Yes. > > Otherwise we will lose the value calculated by core via > qdisc_calculate_pkt_len(). qdisc_calculate_pkt_len only be cablled in dev_xmit_skb and qdisc_enqueue. If all the fragment will pass those before defrag, it will caculate correctly. If the reassembled packets pass those, it also caculate correctly after we recaculate the pkt_len of qdisc_skb_cb
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index 67504ae..e675e2d 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -676,6 +676,7 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb, u8 family, u16 zone) { enum ip_conntrack_info ctinfo; + struct qdisc_skb_cb cb; struct nf_conn *ct; int err = 0; bool frag; @@ -693,6 +694,7 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb, return err; skb_get(skb); + cb = *qdisc_skb_cb(skb); if (family == NFPROTO_IPV4) { enum ip_defrag_users user = IP_DEFRAG_CONNTRACK_IN + zone; @@ -717,6 +719,7 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb, #endif } + *qdisc_skb_cb(skb) = cb; skb_clear_hash(skb); skb->ignore_df = 1; return err; @@ -1014,6 +1017,7 @@ 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; return retval; drop: