| Message ID | 20190214095848.25990-1-matthias.may@neratec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] rstp: add ability to receive VLAN-tagged BPDUs | expand |
Bleep bloop. Greetings Matthias May via dev, 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: ERROR: Author should not be mailing list. Lines checked: 47, Warnings: 0, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
On 14/02/2019 11:59, 0-day Robot wrote: > Bleep bloop. Greetings Matthias May via dev, 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: > ERROR: Author should not be mailing list. > Lines checked: 47, Warnings: 0, Errors: 1 > > > Please check this out. If you feel there has been an error, please email aconole@bytheb.org > > Thanks, > 0-day Robot > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > What is this supposed to mean? Is the Signed-off-by not enough? BR Matthias
Hi Ben, This is another patch with From: field altered to be ovs-dev@openvswitch.org. [...] From: Matthias May via dev <ovs-dev@openvswitch.org> Reply-To: Matthias May <matthias.may@neratec.com> If the patch gets applied as is, the commit's Author will have the wrong email. Just FYI because before Sriharsha and Neal had the same issue and we haven't identified the root cause back then. Matthias, could you describe to where exactly you sent out the patch? ovs-dev@openvswitch.org or dev@openvswitch.org or something else? Thanks, fbl On Thu, Feb 14, 2019 at 12:18:11PM +0100, Matthias May via dev wrote: > On 14/02/2019 11:59, 0-day Robot wrote: > > Bleep bloop. Greetings Matthias May via dev, 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: > > ERROR: Author should not be mailing list. > > Lines checked: 47, Warnings: 0, Errors: 1 > > > > > > Please check this out. If you feel there has been an error, please email aconole@bytheb.org > > > > Thanks, > > 0-day Robot > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > What is this supposed to mean? > Is the Signed-off-by not enough? > > BR > Matthias > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 14/02/2019 14:34, Flavio Leitner wrote: > > Hi Ben, > > This is another patch with From: field altered to be > ovs-dev@openvswitch.org. > > [...] > From: Matthias May via dev <ovs-dev@openvswitch.org> > Reply-To: Matthias May <matthias.may@neratec.com> > > If the patch gets applied as is, the commit's Author will have the > wrong email. > > Just FYI because before Sriharsha and Neal had the same issue and we > haven't identified the root cause back then. > > Matthias, could you describe to where exactly you sent out the > patch? ovs-dev@openvswitch.org or dev@openvswitch.org or something > else? > > Thanks, > fbl > > > On Thu, Feb 14, 2019 at 12:18:11PM +0100, Matthias May via dev wrote: >> On 14/02/2019 11:59, 0-day Robot wrote: >>> Bleep bloop. Greetings Matthias May via dev, 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: >>> ERROR: Author should not be mailing list. >>> Lines checked: 47, Warnings: 0, Errors: 1 >>> >>> >>> Please check this out. If you feel there has been an error, please email aconole@bytheb.org >>> >>> Thanks, >>> 0-day Robot >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >> What is this supposed to mean? >> Is the Signed-off-by not enough? >> >> BR >> Matthias >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > Hi Flavio Some info: * git version 2.20.1 * (Reproduced) trace of what I entered: ``` maym@CHD500279:~/git/ovs$ git send-email send-email/ --no-chain-reply send-email/0001-rstp-add-ability-to-receive-VLAN-tagged-BPDUs.patch To whom should the emails be sent (if anyone)? ovs-dev@openvswitch.org Message-ID to be used as In-Reply-To for the first email (if any)? (mbox) Adding cc: Matthias May <matthias.may@neratec.com> from line 'From: Matthias May <matthias.may@neratec.com>' (body) Adding cc: Matthias May <matthias.may@neratec.com> from line 'Signed-off-by: Matthias May <matthias.may@neratec.com>' From: Matthias May <matthias.may@neratec.com> To: ovs-dev@openvswitch.org Cc: Matthias May <matthias.may@neratec.com> Subject: [PATCH] rstp: add ability to receive VLAN-tagged BPDUs Date: Thu, 14 Feb 2019 15:01:34 +0100 Message-Id: <20190214140134.16754-1-matthias.may@neratec.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit The Cc list above has been expanded by additional addresses found in the patch commit message. By default send-email prompts before sending whenever this occurs. This behavior is controlled by the sendemail.confirm configuration setting. For additional information, run 'git send-email --help'. To retain the current behavior, but squelch this message, run 'git config --global sendemail.confirm auto'. Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y ``` This obviously has a different Message-Id then the mail sent before. BR Matthias
On 14/02/2019 14:34, Flavio Leitner wrote: > > Hi Ben, > > This is another patch with From: field altered to be > ovs-dev@openvswitch.org. > > [...] > From: Matthias May via dev <ovs-dev@openvswitch.org> > Reply-To: Matthias May <matthias.may@neratec.com> > > If the patch gets applied as is, the commit's Author will have the > wrong email. > > Just FYI because before Sriharsha and Neal had the same issue and we > haven't identified the root cause back then. > > Matthias, could you describe to where exactly you sent out the > patch? ovs-dev@openvswitch.org or dev@openvswitch.org or something > else? > > Thanks, > fbl > > *thread snipped* Maybe this is related to the SRS settings on mailman. Looks to me a bit like "munge" is used instead of "wrap". BR Matthias
On Thu, Feb 14, 2019 at 03:14:26PM +0100, Matthias May wrote: > On 14/02/2019 14:34, Flavio Leitner wrote: > > > > Hi Ben, > > > > This is another patch with From: field altered to be > > ovs-dev@openvswitch.org. > > > > [...] > > From: Matthias May via dev <ovs-dev@openvswitch.org> > > Reply-To: Matthias May <matthias.may@neratec.com> > > > > If the patch gets applied as is, the commit's Author will have the > > wrong email. > > > > Just FYI because before Sriharsha and Neal had the same issue and we > > haven't identified the root cause back then. > > > > Matthias, could you describe to where exactly you sent out the > > patch? ovs-dev@openvswitch.org or dev@openvswitch.org or something > > else? > > > > Thanks, > > fbl > > > > > *thread snipped* > Maybe this is related to the SRS settings on mailman. > Looks to me a bit like "munge" is used instead of "wrap". I guess that you're talking about the mailman setting for "from_is_list (general): Replace the From: header address with the list's posting address to mitigate issues stemming from the original From: domain's DMARC or similar policies." This is set to "no", to avoid changing From: addresses at all.
On Thu, Feb 14, 2019 at 03:06:33PM +0100, Matthias May via dev wrote: > On 14/02/2019 14:34, Flavio Leitner wrote: > > Matthias, could you describe to where exactly you sent out the > > patch? ovs-dev@openvswitch.org or dev@openvswitch.org or something > > else? > Some info: > * git version 2.20.1 > * (Reproduced) trace of what I entered: > ``` > maym@CHD500279:~/git/ovs$ git send-email send-email/ --no-chain-reply > send-email/0001-rstp-add-ability-to-receive-VLAN-tagged-BPDUs.patch > To whom should the emails be sent (if anyone)? ovs-dev@openvswitch.org > > Message-ID to be used as In-Reply-To for the first email (if any)? > > (mbox) Adding cc: Matthias May <matthias.may@neratec.com> from line 'From: Matthias May <matthias.may@neratec.com>' > (body) Adding cc: Matthias May <matthias.may@neratec.com> from line 'Signed-off-by: Matthias May <matthias.may@neratec.com>' > > From: Matthias May <matthias.may@neratec.com> > To: ovs-dev@openvswitch.org > Cc: Matthias May <matthias.may@neratec.com> > Subject: [PATCH] rstp: add ability to receive VLAN-tagged BPDUs > Date: Thu, 14 Feb 2019 15:01:34 +0100 > Message-Id: <20190214140134.16754-1-matthias.may@neratec.com> > X-Mailer: git-send-email 2.20.1 > MIME-Version: 1.0 > Content-Transfer-Encoding: 8bit > > The Cc list above has been expanded by additional > addresses found in the patch commit message. By default > send-email prompts before sending whenever this occurs. > This behavior is controlled by the sendemail.confirm > configuration setting. > > For additional information, run 'git send-email --help'. > To retain the current behavior, but squelch this message, > run 'git config --global sendemail.confirm auto'. > > Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y > > ``` > > This obviously has a different Message-Id then the mail sent before. Do you mind to send the same patch to dev@openvswitch.org to see if that happens again? I don't know how the list is configured but so far it only happened with ovs-dev@ and not with dev@. Thanks, fbl
On Thu, Feb 14, 2019 at 10:58:48AM +0100, Matthias May via dev wrote: > There are switches which allow to transmit their BPDUs VLAN-tagged. > With this change OVS is able to receive VLAN-tagged BPDUs, but still > transmits its own BPDUs untagged. > This was tested against Westermo RFI-207-F4G-T3G. > > Signed-off-by: Matthias May <matthias.may@neratec.com> Thanks for the patch. To me, it seems really odd to treat packets with and without an arbitrary VLAN header the same way. I could see it if the VLAN header had VID 0 or 1 or some other specified value, but it seems unusual to ignore it entirely. Is this standardized or a de facto standard of some kind?
On 14/02/2019 20:17, Ben Pfaff wrote: > On Thu, Feb 14, 2019 at 10:58:48AM +0100, Matthias May via dev wrote: >> There are switches which allow to transmit their BPDUs VLAN-tagged. >> With this change OVS is able to receive VLAN-tagged BPDUs, but still >> transmits its own BPDUs untagged. >> This was tested against Westermo RFI-207-F4G-T3G. >> >> Signed-off-by: Matthias May <matthias.may@neratec.com> > > Thanks for the patch. > > To me, it seems really odd to treat packets with and without an > arbitrary VLAN header the same way. I could see it if the VLAN header > had VID 0 or 1 or some other specified value, but it seems unusual to > ignore it entirely. Is this standardized or a de facto standard of some > kind? > I totally agree. To me a VLAN header has nothing lost on a BPDU of a (R)STP frame, simply because (R)STP is not per VLAN. However the fact is that there are switches which are transmitting frames on a VLAN. With this change we simply ignore the VLAN header if is present. The meaning of the BPDU doesn't cheange. The provided information still is not per VLAN and applies to all ports the same. This patch does not add the ability to transmit VLAN tagged BPDUs for the same reasoning above: RSTP/STP is not supposed to be per VLAN. I was thinking about adding to the patch that one can specify a VLAN via config and only BPDUs with the configured VLAN are accepted. I guess this is what you propose: only accept vlan tagged BPDUs on a specified VLAN. Having such a config-parameter would also enable to transmit the BPDUs VLAN tagged. But I'm still of the opinion that this only suggests that one could have an (R)STP tree per VLAN. BR Matthias
On Fri, Feb 15, 2019 at 12:27:11AM +0100, Matthias May wrote: > On 14/02/2019 20:17, Ben Pfaff wrote: > > On Thu, Feb 14, 2019 at 10:58:48AM +0100, Matthias May via dev wrote: > >> There are switches which allow to transmit their BPDUs VLAN-tagged. > >> With this change OVS is able to receive VLAN-tagged BPDUs, but still > >> transmits its own BPDUs untagged. > >> This was tested against Westermo RFI-207-F4G-T3G. > >> > >> Signed-off-by: Matthias May <matthias.may@neratec.com> > > > > Thanks for the patch. > > > > To me, it seems really odd to treat packets with and without an > > arbitrary VLAN header the same way. I could see it if the VLAN header > > had VID 0 or 1 or some other specified value, but it seems unusual to > > ignore it entirely. Is this standardized or a de facto standard of some > > kind? > > > > I totally agree. > To me a VLAN header has nothing lost on a BPDU of a (R)STP frame, simply > because (R)STP is not per VLAN. > > However the fact is that there are switches which are transmitting > frames on a VLAN. > > With this change we simply ignore the VLAN header if is present. The > meaning of the BPDU doesn't cheange. The provided information still is > not per VLAN and applies to all ports the same. > > This patch does not add the ability to transmit VLAN tagged BPDUs for > the same reasoning above: RSTP/STP is not supposed to be per VLAN. > I was thinking about adding to the patch that one can specify a VLAN via > config and only BPDUs with the configured VLAN are accepted. I guess > this is what you propose: only accept vlan tagged BPDUs on a specified VLAN. > Having such a config-parameter would also enable to transmit the BPDUs > VLAN tagged. But I'm still of the opinion that this only suggests that > one could have an (R)STP tree per VLAN. I see we are basically in agreement, but I'd like more information, if you have it. Do the switches that transmit RSTP on a VLAN transmit it on a particular VLAN like 0 or 1? (Maybe they are transmitting them as priority-tagged frames for some reason?) If so, then it would be possible to accept just that VLAN.
On 14/02/2019 16:51, Ben Pfaff wrote: > On Thu, Feb 14, 2019 at 03:14:26PM +0100, Matthias May wrote: >> On 14/02/2019 14:34, Flavio Leitner wrote: >>> >>> Hi Ben, >>> >>> This is another patch with From: field altered to be >>> ovs-dev@openvswitch.org. >>> >>> [...] >>> From: Matthias May via dev <ovs-dev@openvswitch.org> >>> Reply-To: Matthias May <matthias.may@neratec.com> >>> >>> If the patch gets applied as is, the commit's Author will have the >>> wrong email. >>> >>> Just FYI because before Sriharsha and Neal had the same issue and we >>> haven't identified the root cause back then. >>> >>> Matthias, could you describe to where exactly you sent out the >>> patch? ovs-dev@openvswitch.org or dev@openvswitch.org or something >>> else? >>> >>> Thanks, >>> fbl >>> >>> >> *thread snipped* >> Maybe this is related to the SRS settings on mailman. >> Looks to me a bit like "munge" is used instead of "wrap". > > I guess that you're talking about the mailman setting for "from_is_list > (general): Replace the From: header address with the list's posting > address to mitigate issues stemming from the original From: domain's > DMARC or similar policies." > > This is set to "no", to avoid changing From: addresses at all. > Hi Flavio, Ben Sending the mail via dev instead of ovs-dev didn't seem to make a difference. I looked at the DMARC entry of neratec.com https://mxtoolbox.com/SuperTool.aspx?action=dmarc%3aneratec.com&run=toolpage The policy is set to quarantine. I went back in the archive and checked the other occurrences of the "Author should not be mailing list". All of the domains involved have their policy set to "quarantine". So I guess this is the root cause. I seems that the used mailman is on version 2.1.12 According to https://wiki.list.org/DEV/DMARC starting with 2.1.16/18 there are more options to handle this. Maybe it's time to update? BR Matthias
On 15/02/2019 01:28, Ben Pfaff wrote: > On Fri, Feb 15, 2019 at 12:27:11AM +0100, Matthias May wrote: >> On 14/02/2019 20:17, Ben Pfaff wrote: >>> On Thu, Feb 14, 2019 at 10:58:48AM +0100, Matthias May via dev wrote: >>>> There are switches which allow to transmit their BPDUs VLAN-tagged. >>>> With this change OVS is able to receive VLAN-tagged BPDUs, but still >>>> transmits its own BPDUs untagged. >>>> This was tested against Westermo RFI-207-F4G-T3G. >>>> >>>> Signed-off-by: Matthias May <matthias.may@neratec.com> >>> >>> Thanks for the patch. >>> >>> To me, it seems really odd to treat packets with and without an >>> arbitrary VLAN header the same way. I could see it if the VLAN header >>> had VID 0 or 1 or some other specified value, but it seems unusual to >>> ignore it entirely. Is this standardized or a de facto standard of some >>> kind? >>> >> >> I totally agree. >> To me a VLAN header has nothing lost on a BPDU of a (R)STP frame, simply >> because (R)STP is not per VLAN. >> >> However the fact is that there are switches which are transmitting >> frames on a VLAN. >> >> With this change we simply ignore the VLAN header if is present. The >> meaning of the BPDU doesn't cheange. The provided information still is >> not per VLAN and applies to all ports the same. >> >> This patch does not add the ability to transmit VLAN tagged BPDUs for >> the same reasoning above: RSTP/STP is not supposed to be per VLAN. >> I was thinking about adding to the patch that one can specify a VLAN via >> config and only BPDUs with the configured VLAN are accepted. I guess >> this is what you propose: only accept vlan tagged BPDUs on a specified VLAN. >> Having such a config-parameter would also enable to transmit the BPDUs >> VLAN tagged. But I'm still of the opinion that this only suggests that >> one could have an (R)STP tree per VLAN. > > I see we are basically in agreement, but I'd like more information, if > you have it. > > Do the switches that transmit RSTP on a VLAN transmit it on a particular > VLAN like 0 or 1? (Maybe they are transmitting them as priority-tagged > frames for some reason?) If so, then it would be possible to accept > just that VLAN. > Hi Ben The switch I tested against has config options to specify with which VLAN the BPDUs should be transmitted. From the tests I did, for this switch it doesn't matter if the received BPDUs are tagged or not. The VLAN header is simply ignored. I have an open question with the vendor regarding this. I did some digging on the net how other switches handle this. E.g at [1]. From what I get, switches which allow tagged BPDUs simply ignore the header. I started reading the 802.1d-2004 and 802.1q-2018, but so far haven't found anything which specifies how this situation should be handled. BR Matthias [1] https://community.extremenetworks.com/extremeswitching-eos-230140/what-happens-if-an-s-series-receives-tagged-rstp-bpdus-7765211
On Mon, Feb 18, 2019 at 12:07:54PM +0100, Matthias May wrote: > On 15/02/2019 01:28, Ben Pfaff wrote: > > On Fri, Feb 15, 2019 at 12:27:11AM +0100, Matthias May wrote: > >> On 14/02/2019 20:17, Ben Pfaff wrote: > >>> On Thu, Feb 14, 2019 at 10:58:48AM +0100, Matthias May via dev wrote: > >>>> There are switches which allow to transmit their BPDUs VLAN-tagged. > >>>> With this change OVS is able to receive VLAN-tagged BPDUs, but still > >>>> transmits its own BPDUs untagged. > >>>> This was tested against Westermo RFI-207-F4G-T3G. > >>>> > >>>> Signed-off-by: Matthias May <matthias.may@neratec.com> > >>> > >>> Thanks for the patch. > >>> > >>> To me, it seems really odd to treat packets with and without an > >>> arbitrary VLAN header the same way. I could see it if the VLAN header > >>> had VID 0 or 1 or some other specified value, but it seems unusual to > >>> ignore it entirely. Is this standardized or a de facto standard of some > >>> kind? > >>> > >> > >> I totally agree. > >> To me a VLAN header has nothing lost on a BPDU of a (R)STP frame, simply > >> because (R)STP is not per VLAN. > >> > >> However the fact is that there are switches which are transmitting > >> frames on a VLAN. > >> > >> With this change we simply ignore the VLAN header if is present. The > >> meaning of the BPDU doesn't cheange. The provided information still is > >> not per VLAN and applies to all ports the same. > >> > >> This patch does not add the ability to transmit VLAN tagged BPDUs for > >> the same reasoning above: RSTP/STP is not supposed to be per VLAN. > >> I was thinking about adding to the patch that one can specify a VLAN via > >> config and only BPDUs with the configured VLAN are accepted. I guess > >> this is what you propose: only accept vlan tagged BPDUs on a specified VLAN. > >> Having such a config-parameter would also enable to transmit the BPDUs > >> VLAN tagged. But I'm still of the opinion that this only suggests that > >> one could have an (R)STP tree per VLAN. > > > > I see we are basically in agreement, but I'd like more information, if > > you have it. > > > > Do the switches that transmit RSTP on a VLAN transmit it on a particular > > VLAN like 0 or 1? (Maybe they are transmitting them as priority-tagged > > frames for some reason?) If so, then it would be possible to accept > > just that VLAN. > > > > Hi Ben > The switch I tested against has config options to specify with which VLAN the BPDUs should be transmitted. > From the tests I did, for this switch it doesn't matter if the received BPDUs are tagged or not. The VLAN header is > simply ignored. > I have an open question with the vendor regarding this. > > I did some digging on the net how other switches handle this. E.g at [1]. > From what I get, switches which allow tagged BPDUs simply ignore the header. > > I started reading the 802.1d-2004 and 802.1q-2018, but so far haven't found anything which specifies how this situation > should be handled. I guess networking doesn't always make total sense. It's a shame. I applied the patch to master, thanks!
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index acd4817c2..7830fee2e 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1771,9 +1771,12 @@ xport_rstp_should_manage_bpdu(const struct xport *xport) return rstp_should_manage_bpdu(xport_get_rstp_port_state(xport)); } +#define LEN_WITHOUT_VLAN (ETH_HEADER_LEN + LLC_HEADER_LEN) +#define LEN_WITH_VLAN (VLAN_HEADER_LEN + LEN_WITHOUT_VLAN) static void rstp_process_packet(const struct xport *xport, const struct dp_packet *packet) { + int len; struct dp_packet payload = *packet; struct eth_header *eth = dp_packet_data(&payload); @@ -1787,7 +1790,9 @@ rstp_process_packet(const struct xport *xport, const struct dp_packet *packet) dp_packet_set_size(&payload, ntohs(eth->eth_type) + ETH_HEADER_LEN); } - if (dp_packet_try_pull(&payload, ETH_HEADER_LEN + LLC_HEADER_LEN)) { + /* Pull a bit less payload when the BPDU is enveloped in a VLAN header */ + len = (ntohs(eth->eth_type) == 0x8100) ? LEN_WITH_VLAN : LEN_WITHOUT_VLAN; + if (dp_packet_try_pull(&payload, len)) { rstp_port_received_bpdu(xport->rstp_port, dp_packet_data(&payload), dp_packet_size(&payload)); }
There are switches which allow to transmit their BPDUs VLAN-tagged. With this change OVS is able to receive VLAN-tagged BPDUs, but still transmits its own BPDUs untagged. This was tested against Westermo RFI-207-F4G-T3G. Signed-off-by: Matthias May <matthias.may@neratec.com> --- ofproto/ofproto-dpif-xlate.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)