[PATCH net-next] net: ovs: fix ovs_drop_reasons error

[PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Menglong Dong]

I'm sure if I understand it correctly, but it seems that there is
something wrong with ovs_drop_reasons.

ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but
OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that
ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION".

Fix this by initializing ovs_drop_reasons with index.

Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason")
Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 net/openvswitch/datapath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 99d72543abd3..249210958f0b 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -2706,7 +2706,7 @@ static struct pernet_operations ovs_net_ops = {
 };
 
 static const char * const ovs_drop_reasons[] = {
-#define S(x)	(#x),
+#define S(x)	[(x) & ~SKB_DROP_REASON_SUBSYS_MASK] = (#x),
 	OVS_DROP_REASONS(S)
 #undef S
 };
-- 
2.39.2

Re: [PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Jakub Kicinski]

On Thu, 15 Aug 2024 20:22:45 +0800 Menglong Dong wrote:
> I'm sure if I understand it correctly, but it seems that there is
> something wrong with ovs_drop_reasons.
> 
> ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but
> OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that
> ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION".
> 
> Fix this by initializing ovs_drop_reasons with index.
> 
> Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason")
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>

Could you include output? Presumably from drop monitor?
I think it should go to net rather than net-next.
-- 
pw-bot: cr

Re: [PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Menglong Dong]

On Sat, Aug 17, 2024 at 9:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 15 Aug 2024 20:22:45 +0800 Menglong Dong wrote:
> > I'm sure if I understand it correctly, but it seems that there is
> > something wrong with ovs_drop_reasons.
> >
> > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but
> > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that
> > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION".
> >
> > Fix this by initializing ovs_drop_reasons with index.
> >
> > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason")
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
>
> Could you include output? Presumably from drop monitor?
> I think it should go to net rather than net-next.

I have not tested it yet. I just copy all the code for drop reason
subsystem from openvswitch to vxlan, and this happens in
the vxlan code.

I'm on vacation right now, and I'll test it out next Monday.

Thanks!
Menglong Dong

> --
> pw-bot: cr

Re: [PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Menglong Dong]

On Sat, Aug 17, 2024 at 9:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 15 Aug 2024 20:22:45 +0800 Menglong Dong wrote:
> > I'm sure if I understand it correctly, but it seems that there is
> > something wrong with ovs_drop_reasons.
> >
> > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but
> > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that
> > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION".
> >
> > Fix this by initializing ovs_drop_reasons with index.
> >
> > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason")
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
>
> Could you include output? Presumably from drop monitor?

I think I'm right. I'm not familiar with ovs, and it's hard to
create a drop case for me. So, I did some modification to
ovs_vport_receive link this:

@@ -510,6 +511,9 @@ int ovs_vport_receive(struct vport *vport, struct
sk_buff *skb,
                tun_info = NULL;
        }

+       ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+       return -ENOMEM;
+
        /* Extract flow from 'skb' into 'key'. */
        error = ovs_flow_key_extract(tun_info, skb, &key);
        if (unlikely(error)) {

In the drop watch, I can have the output like this:

drop at: ovs_vport_receive+0x78/0xc0 [openvswitc (0xffffffffc068fa78)
origin: software
input port ifindex: 18
timestamp: Sun Aug 18 03:28:00 2024 142999108 nsec
protocol: 0x86dd
length: 70
original length: 70
drop reason: OVS_DROP_ACTION_ERROR

Obviously, the output is wrong, just like what I suspect.

> I think it should go to net rather than net-next.

Should I resend this patch to the net branch?

Thanks!
Menglong Dong

> --
> pw-bot: cr

Re: [PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Jakub Kicinski]

On Sun, 18 Aug 2024 11:35:48 +0800 Menglong Dong wrote:
> > I think it should go to net rather than net-next.  
> 
> Should I resend this patch to the net branch?

Yes please! And with the results of your experiment added to the commit
message.

Re: [PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Adrián Moreno]

On Thu, Aug 15, 2024 at 08:22:45PM GMT, Menglong Dong wrote:
> I'm sure if I understand it correctly, but it seems that there is
> something wrong with ovs_drop_reasons.
>
> ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but
> OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that
> ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION".
>
> Fix this by initializing ovs_drop_reasons with index.
>
> Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason")
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>

The patch looks good to me. Also, tested and verified that,
without the patch, adding flow to drop packets results in:

drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97)
origin: software
input port ifindex: 8
timestamp: Tue Aug 20 10:19:17 2024 859853461 nsec
protocol: 0x800
length: 98
original length: 98
drop reason: OVS_DROP_ACTION_ERROR

With the patch, the same results in:

drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97)
origin: software
input port ifindex: 8
timestamp: Tue Aug 20 10:16:13 2024 475856608 nsec
protocol: 0x800
length: 98
original length: 98
drop reason: OVS_DROP_LAST_ACTION

Tested-by: Adrian Moreno <amorenoz@redhat.com>
Reviewed-by: Adrian Moreno <amorenoz@redhat.com>

Re: [PATCH net-next] net: ovs: fix ovs_drop_reasons error

[Menglong Dong]

On Tue, Aug 20, 2024 at 10:23 PM Adrián Moreno <amorenoz@redhat.com> wrote:
>
> On Thu, Aug 15, 2024 at 08:22:45PM GMT, Menglong Dong wrote:
> > I'm sure if I understand it correctly, but it seems that there is
> > something wrong with ovs_drop_reasons.
> >
> > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but
> > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that
> > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION".
> >
> > Fix this by initializing ovs_drop_reasons with index.
> >
> > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason")
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
>
> The patch looks good to me. Also, tested and verified that,
> without the patch, adding flow to drop packets results in:
>
> drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97)
> origin: software
> input port ifindex: 8
> timestamp: Tue Aug 20 10:19:17 2024 859853461 nsec
> protocol: 0x800
> length: 98
> original length: 98
> drop reason: OVS_DROP_ACTION_ERROR
>
> With the patch, the same results in:
>
> drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97)
> origin: software
> input port ifindex: 8
> timestamp: Tue Aug 20 10:16:13 2024 475856608 nsec
> protocol: 0x800
> length: 98
> original length: 98
> drop reason: OVS_DROP_LAST_ACTION
>
> Tested-by: Adrian Moreno <amorenoz@redhat.com>
> Reviewed-by: Adrian Moreno <amorenoz@redhat.com>
>

Thanks! I'll resend this patch to the net branch
with your testing.