| Message ID | 13c7b29126171310739195264d5e619b62d27f92.1632133123.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: drop ct and dst for the packets toward ingress in act_mirred | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: roid@nvidia.com; 4 maintainers not CCed: jhs@mojatatu.com roid@nvidia.com jiri@resnulli.us xiyou.wangcong@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 27 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for > those packets that are mirred or redirected to only ingress, not > ingress and egress. Any reason behind this? I think we at least need to reset it when redirecting from ingress to egress as well? That is, when changing directions? Thanks.
On Tue, Sep 21, 2021 at 2:31 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for > > those packets that are mirred or redirected to only ingress, not > > ingress and egress. > > Any reason behind this? I think we at least need to reset it when > redirecting from ingress to egress as well? That is, when changing > directions? For the reason why ct should be reset, it's said in d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror"). The user case is OVS HWOL using TC to do NAT and then redirecting the NATed skb back to the ingress of one local dev, it's ingress only, this patch is more like to minimize the side effect of d09c548dbf3b IF there is. Not sure if it's too much to do for that from ingress to egress. What I was thinking is this should happen on rx path(ingress), like it does in internal_dev_recv() in the OVS kernel module. But IF there is any user case needing doing this for ingress to egress, I would add it. > > Thanks.
On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Tue, Sep 21, 2021 at 2:31 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for > > > those packets that are mirred or redirected to only ingress, not > > > ingress and egress. > > > > Any reason behind this? I think we at least need to reset it when > > redirecting from ingress to egress as well? That is, when changing > > directions? > For the reason why ct should be reset, it's said in > d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror"). > The user case is OVS HWOL using TC to do NAT and then redirecting > the NATed skb back to the ingress of one local dev, it's ingress only, this > patch is more like to minimize the side effect of d09c548dbf3b IF there is. What is the side effect here? Or what is wrong with resetting CT on egress side? > > Not sure if it's too much to do for that from ingress to egress. > What I was thinking is this should happen on rx path(ingress), like it > does in internal_dev_recv() in the OVS kernel module. But IF there is > any user case needing doing this for ingress to egress, I would add it. If that is the case, then this patch is completely unnecessary. So instead of going back and forth, please elaborate on why resetting CT for egress is a problem here. Thanks.
On Wed, Sep 22, 2021 at 11:43 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Tue, Sep 21, 2021 at 2:31 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for > > > > those packets that are mirred or redirected to only ingress, not > > > > ingress and egress. > > > > > > Any reason behind this? I think we at least need to reset it when > > > redirecting from ingress to egress as well? That is, when changing > > > directions? > > For the reason why ct should be reset, it's said in > > d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror"). > > The user case is OVS HWOL using TC to do NAT and then redirecting > > the NATed skb back to the ingress of one local dev, it's ingress only, this > > patch is more like to minimize the side effect of d09c548dbf3b IF there is. > > What is the side effect here? Or what is wrong with resetting CT on > egress side? > > > > > Not sure if it's too much to do for that from ingress to egress. > > What I was thinking is this should happen on rx path(ingress), like it > > does in internal_dev_recv() in the OVS kernel module. But IF there is > > any user case needing doing this for ingress to egress, I would add it. > > If that is the case, then this patch is completely unnecessary. So > instead of going back and forth, please elaborate on why resetting > CT for egress is a problem here. What I'm afraid is: after resetting CT for the packets redirected to egress, the tc rules on the next dev egress that may need these CT will break. I can't give a use case right now, and just don't want to introduce extra change for which we don't see a use. If you think that's safe, I'm fine to do these resets when the direction is changed.
On Wed, Sep 22, 2021 at 01:17:05PM +0800, Xin Long wrote: > On Wed, Sep 22, 2021 at 11:43 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Tue, Sep 21, 2021 at 2:31 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for > > > > > those packets that are mirred or redirected to only ingress, not > > > > > ingress and egress. > > > > > > > > Any reason behind this? I think we at least need to reset it when > > > > redirecting from ingress to egress as well? That is, when changing > > > > directions? > > > For the reason why ct should be reset, it's said in > > > d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror"). > > > The user case is OVS HWOL using TC to do NAT and then redirecting > > > the NATed skb back to the ingress of one local dev, it's ingress only, this > > > patch is more like to minimize the side effect of d09c548dbf3b IF there is. > > > > What is the side effect here? Or what is wrong with resetting CT on > > egress side? > > > > > > > > Not sure if it's too much to do for that from ingress to egress. > > > What I was thinking is this should happen on rx path(ingress), like it > > > does in internal_dev_recv() in the OVS kernel module. But IF there is > > > any user case needing doing this for ingress to egress, I would add it. > > > > If that is the case, then this patch is completely unnecessary. So > > instead of going back and forth, please elaborate on why resetting > > CT for egress is a problem here. > What I'm afraid is: after resetting CT for the packets redirected to egress, > the tc rules on the next dev egress that may need these CT will break. This shouldn't happen. Well, it's not expected from OVS use case POV, and that would probably be a nightmare to maintain (without something like OVS), considering packets can be directed to egress from multiple places and they would have different starting points then. > > I can't give a use case right now, and just don't want to introduce extra > change for which we don't see a use. If you think that's safe, I'm fine > to do these resets when the direction is changed. I don't see why it wouldn't be safe.
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c index d64b0eeccbe4..46dff1f1e7c8 100644 --- a/net/sched/act_mirred.c +++ b/net/sched/act_mirred.c @@ -205,14 +205,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla, static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb) { - int err; - if (!want_ingress) - err = tcf_dev_queue_xmit(skb, dev_queue_xmit); - else - err = netif_receive_skb(skb); + return tcf_dev_queue_xmit(skb, dev_queue_xmit); - return err; + nf_reset_ct(skb); + + return netif_receive_skb(skb); } static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a, @@ -271,9 +269,6 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a, goto out; } - /* All mirred/redirected skbs should clear previous ct info */ - nf_reset_ct(skb2); - want_ingress = tcf_mirred_act_wants_ingress(m_eaction); expects_nh = want_ingress || !m_mac_header_xmit;
nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for those packets that are mirred or redirected to only ingress, not ingress and egress. This patch is to move nf_reset_ct() to tcf_mirred_forward() and to be called only when want_ingress is true. Fixes: d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror") Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sched/act_mirred.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)