| Message ID | 20191103091153.212675-1-roid@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-xlate: Prevent duplicating of traffic to a mirror port | expand |
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
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 >
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.
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.
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) {