[ovs-dev] ofproto-dpif-xlate: Prevent duplicating of traffic to a mirror port
diff mbox series

Message ID 20191103091153.212675-1-roid@mellanox.com
State New
Headers show
Series
  • [ovs-dev] ofproto-dpif-xlate: Prevent duplicating of traffic to a mirror port
Related show

Commit Message

Roi Dayan Nov. 3, 2019, 9:11 a.m. UTC
From: Dmytro Linkin <dmitrolin@mellanox.com>

Currently ofproto design disallow duplicating output packet on forwarding
and mirroring to/from same ovs port. Next scenario reveal lack of design:
1. Send ping between regular ovs ports (VFs, for ex.), stop it.
2. While rule still exist, make mirror for one of the ports.
Prevent duplicating of traffic to a mirror port.

Fixes: 86e2dcddce85 ("dpif-xlate: Snoop multicast packets and send them properly")
Signed-off-by: Dmytro Linkin <dmitrolin@mellanox.com>
Acked-by: Roi Dayan <roid@mellanox.com>
---
 ofproto/ofproto-dpif-xlate.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

0-day Robot Nov. 3, 2019, 10 a.m. UTC | #1
Bleep bloop.  Greetings Roi Dayan, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#31 FILE: ofproto/ofproto-dpif-xlate.c:3125:
                    xlate_report_error(ctx, "dropping packet received on port %s, "

WARNING: Line is 85 characters long (recommended limit is 79)
#32 FILE: ofproto/ofproto-dpif-xlate.c:3126:
                                       "which is reserved exclusively for mirroring",

Lines checked: 45, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Roi Dayan Nov. 3, 2019, 1:19 p.m. UTC | #2
On 2019-11-03 12:00 PM, 0-day Robot wrote:
> Bleep bloop.  Greetings Roi Dayan, I am a robot and I have tried out your patch.
> Thanks for your contribution.
> 
> I encountered some error that I wasn't expecting.  See the details below.
> 
> 
> checkpatch:
> WARNING: Line is 83 characters long (recommended limit is 79)
> #31 FILE: ofproto/ofproto-dpif-xlate.c:3125:
>                     xlate_report_error(ctx, "dropping packet received on port %s, "
> 
> WARNING: Line is 85 characters long (recommended limit is 79)
> #32 FILE: ofproto/ofproto-dpif-xlate.c:3126:
>                                        "which is reserved exclusively for mirroring",
> 
> Lines checked: 45, Warnings: 2, Errors: 0
> 

It's the same prints taken from little up in the function when the same drop happens.
so to be consistent it was left the same.

> 
> Please check this out.  If you feel there has been an error, please email aconole@redhat.com
> 
> Thanks,
> 0-day Robot
>
Ben Pfaff Dec. 2, 2019, 10:47 p.m. UTC | #3
On Sun, Nov 03, 2019 at 11:11:53AM +0200, Roi Dayan wrote:
> From: Dmytro Linkin <dmitrolin@mellanox.com>
> 
> Currently ofproto design disallow duplicating output packet on forwarding
> and mirroring to/from same ovs port. Next scenario reveal lack of design:
> 1. Send ping between regular ovs ports (VFs, for ex.), stop it.
> 2. While rule still exist, make mirror for one of the ports.
> Prevent duplicating of traffic to a mirror port.
> 
> Fixes: 86e2dcddce85 ("dpif-xlate: Snoop multicast packets and send them properly")
> Signed-off-by: Dmytro Linkin <dmitrolin@mellanox.com>
> Acked-by: Roi Dayan <roid@mellanox.com>

Thanks for the patch!

I don't think that the following message is correct, because the tests
here are not concerned with the input port.  I think that this message
should be dropped:
> +                if (ctx->xin->packet != NULL) {
> +                    xlate_report_error(ctx, "dropping packet received on port %s, "
> +                                       "which is reserved exclusively for mirroring",
> +                                       mac_xbundle->name);
> +                }

This one might better be phrased as "learned port" rather than "output
port":

> +                xlate_report(ctx, OFT_WARN,
> +                             "output port is a mirror port, dropping");
> +                return;
> +            }

Thanks,

Ben.
Roi Dayan Dec. 3, 2019, 1:48 p.m. UTC | #4
On 2019-12-03 12:47 AM, Ben Pfaff wrote:
> On Sun, Nov 03, 2019 at 11:11:53AM +0200, Roi Dayan wrote:
>> From: Dmytro Linkin <dmitrolin@mellanox.com>
>>
>> Currently ofproto design disallow duplicating output packet on forwarding
>> and mirroring to/from same ovs port. Next scenario reveal lack of design:
>> 1. Send ping between regular ovs ports (VFs, for ex.), stop it.
>> 2. While rule still exist, make mirror for one of the ports.
>> Prevent duplicating of traffic to a mirror port.
>>
>> Fixes: 86e2dcddce85 ("dpif-xlate: Snoop multicast packets and send them properly")
>> Signed-off-by: Dmytro Linkin <dmitrolin@mellanox.com>
>> Acked-by: Roi Dayan <roid@mellanox.com>
> 
> Thanks for the patch!
> 
> I don't think that the following message is correct, because the tests
> here are not concerned with the input port.  I think that this message
> should be dropped:
>> +                if (ctx->xin->packet != NULL) {
>> +                    xlate_report_error(ctx, "dropping packet received on port %s, "
>> +                                       "which is reserved exclusively for mirroring",
>> +                                       mac_xbundle->name);
>> +                }
> 
> This one might better be phrased as "learned port" rather than "output
> port":
> 
>> +                xlate_report(ctx, OFT_WARN,
>> +                             "output port is a mirror port, dropping");
>> +                return;
>> +            }
> 
> Thanks,
> 
> Ben.
> 

ok thanks. we'll send v2.

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f92cb62c80ce..935a44dd07c2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3118,6 +3118,19 @@  xlate_normal(struct xlate_ctx *ctx)
 
         if (mac_port) {
             struct xbundle *mac_xbundle = xbundle_lookup(ctx->xcfg, mac_port);
+
+            /* Drop frames if output port is a mirror port. */
+            if (mac_xbundle && xbundle_mirror_out(ctx->xbridge, mac_xbundle)) {
+                if (ctx->xin->packet != NULL) {
+                    xlate_report_error(ctx, "dropping packet received on port %s, "
+                                       "which is reserved exclusively for mirroring",
+                                       mac_xbundle->name);
+                }
+                xlate_report(ctx, OFT_WARN,
+                             "output port is a mirror port, dropping");
+                return;
+            }
+
             if (mac_xbundle
                 && mac_xbundle != in_xbundle
                 && mac_xbundle->ofbundle != in_xbundle->ofbundle) {