| Message ID | 20210225121835.3864036-1-olteanv@gmail.com |
|---|---|
| Headers | show |
| Series | Fixes for NXP ENETC driver | expand |
On Thu, Feb 25, 2021 at 11:52:19PM +0100, Andrew Lunn wrote: > On Thu, Feb 25, 2021 at 02:18:32PM +0200, Vladimir Oltean wrote: > > @@ -327,8 +329,8 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd, > > { > > u32 lo, hi, tstamp_lo; > > > > - lo = enetc_rd(hw, ENETC_SICTR0); > > - hi = enetc_rd(hw, ENETC_SICTR1); > > + lo = enetc_rd_hot(hw, ENETC_SICTR0); > > + hi = enetc_rd_hot(hw, ENETC_SICTR1); > > tstamp_lo = le32_to_cpu(txbd->wb.tstamp); > > if (lo <= tstamp_lo) > > hi -= 1; > > Hi Vladimir > > This change is not obvious, and there is no mention of it in the > commit message. Please could you explain it. I guess it is to do with > enetc_get_tx_tstamp() being called with the MDIO lock held now, when > it was not before? I realize this is an uncharacteristically short commit message and I'm sorry for that, if needed I can resend. Your assumption is correct, the new call path is: enetc_msix -> napi_schedule -> enetc_poll -> enetc_lock_mdio -> enetc_clean_tx_ring -> enetc_get_tx_tstamp -> enetc_clean_rx_ring -> enetc_unlock_mdio The 'hot' accessors are for normal, 'unlocked' register reads and writes, while enetc_rd contains enetc_lock_mdio, followed by the actual read, followed by enetc_unlock_mdio. The goal is to eventually get rid of all the _hot stuff and always take the lock from the top level, this would allow us to do more register read/write batching and that would amortize the cost of the locking overall.
On Thu, 25 Feb 2021 14:18:34 +0200 Vladimir Oltean wrote: > Quoting from the blamed commit: > > In promiscuous mode, it is more intuitive that all traffic is received, > including VLAN tagged traffic. It appears that it is necessary to set > the flag in PSIPVMR for that to be the case, so VLAN promiscuous mode is > also temporarily enabled. On exit from promiscuous mode, the setting > made by ethtool is restored. > > Intuitive or not, there isn't any definition issued by a standards body > which says that promiscuity has anything to do with VLAN filtering - it > only has to do with accepting packets regardless of destination MAC address. > > In fact people are already trying to use this misunderstanding/bug of > the enetc driver as a justification to transform promiscuity into > something it never was about: accepting every packet (maybe that would > be the "rx-all" netdev feature?): > https://lore.kernel.org/netdev/20201110153958.ci5ekor3o2ekg3ky@ipetronik.com/ > > So we should avoid that and delete the bogus logic in enetc. I don't understand what you're fixing tho. Are we trying to establish vlan-filter-on as the expected behavior?
On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote: > I don't understand what you're fixing tho. > > Are we trying to establish vlan-filter-on as the expected behavior? What I'm fixing is unexpected behavior, according to the applicable standards I could find. If I don't mark this change as a bug fix but as a simple patch, somebody could claim it's a regression, since promiscuity used to be enough to see packets with unknown VLANs, and now it no longer is...
On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote: > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote: > > I don't understand what you're fixing tho. > > > > Are we trying to establish vlan-filter-on as the expected behavior? > > What I'm fixing is unexpected behavior, according to the applicable > standards I could find. If I don't mark this change as a bug fix but as > a simple patch, somebody could claim it's a regression, since promiscuity > used to be enough to see packets with unknown VLANs, and now it no > longer is... Can we take it into net-next? What's your feeling on that option?
On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote: > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote: > > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote: > > > I don't understand what you're fixing tho. > > > > > > Are we trying to establish vlan-filter-on as the expected behavior? > > > > What I'm fixing is unexpected behavior, according to the applicable > > standards I could find. If I don't mark this change as a bug fix but as > > a simple patch, somebody could claim it's a regression, since promiscuity > > used to be enough to see packets with unknown VLANs, and now it no > > longer is... > > Can we take it into net-next? What's your feeling on that option? I see how you can view this patch as pointless, but there is some context to it. It isn't just for tcpdump/debugging, instead NXP has some TSN use cases which involve some asymmetric tc-vlan rules, which is how I arrived at this topic in the first place. I've already established that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off: https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/ and that's what we recommend doing, but while adding the support for rx-vlan-filter in enetc I accidentally created another possibility for this to work on enetc, by turning IFF_PROMISC on. This is not portable, so if somebody develops a solution based on that in parallel, it will most certainly break on other non-enetc drivers. NXP has not released a kernel based on the v5.10 stable yet, so there is still time to change the behavior, but if this goes in through net-next, the apparent regression will only be visible when the next LTS comes around (whatever the number of that might be). Now, I'm going to backport this to the NXP v5.10 anyway, so that's not an issue, but there will still be the mild annoyance that the upstream v5.10 will behave differently in this regard compared to the NXP v5.10, which is again a point of potential confusion, but that seems to be out of my control. So if you're still "yeah, don't care", then I guess I'm ok with leaving things alone on stable kernels.
On Sat, 27 Feb 2021 02:16:51 +0200 Vladimir Oltean wrote: > On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote: > > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote: > > > What I'm fixing is unexpected behavior, according to the applicable > > > standards I could find. If I don't mark this change as a bug fix but as > > > a simple patch, somebody could claim it's a regression, since promiscuity > > > used to be enough to see packets with unknown VLANs, and now it no > > > longer is... > > > > Can we take it into net-next? What's your feeling on that option? > > I see how you can view this patch as pointless, but there is some > context to it. It isn't just for tcpdump/debugging, instead NXP has some > TSN use cases which involve some asymmetric tc-vlan rules, which is how > I arrived at this topic in the first place. I've already established > that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off: > https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/ > and that's what we recommend doing, but while adding the support for > rx-vlan-filter in enetc I accidentally created another possibility for > this to work on enetc, by turning IFF_PROMISC on. This is not portable, > so if somebody develops a solution based on that in parallel, it will > most certainly break on other non-enetc drivers. > NXP has not released a kernel based on the v5.10 stable yet, so there is > still time to change the behavior, but if this goes in through net-next, > the apparent regression will only be visible when the next LTS comes > around (whatever the number of that might be). Now, I'm going to > backport this to the NXP v5.10 anyway, so that's not an issue, but there > will still be the mild annoyance that the upstream v5.10 will behave > differently in this regard compared to the NXP v5.10, which is again a > point of potential confusion, but that seems to be out of my control. > > So if you're still "yeah, don't care", then I guess I'm ok with leaving > things alone on stable kernels. I see, so this is indeed of practical importance to NXP. Would you mind re-spinning with an expanded justification?
On Fri, Feb 26, 2021 at 04:45:19PM -0800, Jakub Kicinski wrote: > I see, so this is indeed of practical importance to NXP. > > Would you mind re-spinning with an expanded justification? Sure, I'll do that tomorrow. Thanks.
Am 2021-02-27 01:16, schrieb Vladimir Oltean: > On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote: >> On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote: >> > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote: >> > > I don't understand what you're fixing tho. >> > > >> > > Are we trying to establish vlan-filter-on as the expected behavior? >> > >> > What I'm fixing is unexpected behavior, according to the applicable >> > standards I could find. In the referenced thread you quoted from the IEEE802.3 about the promisc mode. The MAC sublayer may also provide the capability of operating in the promiscuous receive mode. In this mode of operation, the MAC sublayer recognizes and accepts all valid frames, regardless of their Destination Address field values. Your argument was that the standard just talks about disabling the DMAC filter. But was that really the _intention_ of the standard? Does the standard even mention a possible vlan tag? What I mean is: maybe the standard just mention the DMAC because it is the only filtering mechanism in this standard and it's enough to disable it to "accept all valid frames". I was biten by "the NIC drops frames with an unknown VLAN" even if promisc mode was enabled. And IMHO it was quite suprising for me. >> > If I don't mark this change as a bug fix but as >> > a simple patch, somebody could claim it's a regression, since promiscuity >> > used to be enough to see packets with unknown VLANs, and now it no >> > longer is... >> >> Can we take it into net-next? What's your feeling on that option? > > I see how you can view this patch as pointless, but there is some > context to it. It isn't just for tcpdump/debugging, instead NXP has > some > TSN use cases which involve some asymmetric tc-vlan rules, which is how > I arrived at this topic in the first place. I've already established > that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off: > https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/ Wasn't the conclusion that the VID should be added to the filter so it also works with vlan filter enabled? Am I missing another discussion? -michael > and that's what we recommend doing, but while adding the support for > rx-vlan-filter in enetc I accidentally created another possibility for > this to work on enetc, by turning IFF_PROMISC on. This is not portable, > so if somebody develops a solution based on that in parallel, it will > most certainly break on other non-enetc drivers. > NXP has not released a kernel based on the v5.10 stable yet, so there > is > still time to change the behavior, but if this goes in through > net-next, > the apparent regression will only be visible when the next LTS comes > around (whatever the number of that might be). Now, I'm going to > backport this to the NXP v5.10 anyway, so that's not an issue, but > there > will still be the mild annoyance that the upstream v5.10 will behave > differently in this regard compared to the NXP v5.10, which is again a > point of potential confusion, but that seems to be out of my control. > > So if you're still "yeah, don't care", then I guess I'm ok with leaving > things alone on stable kernels.
On Sat, Feb 27, 2021 at 02:18:20PM +0100, Michael Walle wrote: > Am 2021-02-27 01:16, schrieb Vladimir Oltean: > > On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote: > > > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote: > > > > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote: > > > > > I don't understand what you're fixing tho. > > > > > > > > > > Are we trying to establish vlan-filter-on as the expected behavior? > > > > > > > > What I'm fixing is unexpected behavior, according to the applicable > > > > standards I could find. > > In the referenced thread you quoted from the IEEE802.3 about the promisc > mode. > The MAC sublayer may also provide the capability of operating in the > promiscuous receive mode. In this mode of operation, the MAC sublayer > recognizes and accepts all valid frames, regardless of their Destination > Address field values. > > Your argument was that the standard just talks about disabling the DMAC > filter. But was that really the _intention_ of the standard? Does the > standard even mention a possible vlan tag? What I mean is: maybe the > standard just mention the DMAC because it is the only filtering mechanism > in this standard and it's enough to disable it to "accept all valid frames". > > I was biten by "the NIC drops frames with an unknown VLAN" even if > promisc mode was enabled. And IMHO it was quite suprising for me. In short, promiscuity is a function of the MAC sublayer, which is the lower portion of the Data Link Layer (the higher portion being the Logical Link Control layer - LLC). The MAC sublayer is governed by IEEE 802.3, and IEEE 802.1Q does not change anything related to promiscuity, so everything still applies. The MAC sublayer provides its services to the MAC client through something called the MAC service, which uses the following primitives: MA_DATA.request( destination_address, source_address, mac_service_data_unit, frame_check_sequence) to send a frame, and MA_DATA.indication( destination_address, source_address, mac_service_data_unit, frame_check_sequence, ReceiveStatus) to receive a frame. One particular component of the MAC sublayer seems to be called the Internal Sublayer Service (ISS), and this one is defined in IEEE 802.1AC-2016. To be frank, I don't quite grok why there needs to exist this extra layering, but nonetheless, the ISS has some similar service primitives to the MAC sublayer as well, and these are: M_UNITDATA.indication( destination_address, source_address, mac_service_data_unit, priority, drop_eligible, frame_check_sequence, service_access_point_identifier, connection_identifier) M_UNITDATA.request( destination_address, source_address, mac_service_data_unit, priority, drop_eligible, frame_check_sequence, service_access_point_identifier, connection_identifier) where a "unit of data" is basically just very pompous speak for "a frame", I guess. Promiscuity is defined in IEEE 802.3 clause 4A.2.9 Frame reception, which _in_context_ talks about the interface between the MAC client and the MAC sublayer, so that means about the M_UNITDATA.indication or the MA_DATA.indication. Whereas VLAN filtering, as well as adding and removing VLAN tags, is governed by IEEE 802.1Q, as a function of the Enhanced Internal Sublayer Service (EISS), i.e. clause 6.8. In fact, the EISS is just an ISS enhanced for VLAN filtering, as the naming and definition implies. Of course (why not), the EISS has its own service primitives towards its higher-level clients for transmitting and receiving a frame. These are: EM_UNITDATA.request( destination_address, source_address, mac_service_data_unit, priority, drop_eligible, vlan_identifier, frame_check_sequence, service_access_point_identifier, connection_identifier, flow_hash, time_to_live) EM_UNITDATA.indication( destination_address, source_address, mac_service_data_unit, priority, drop_eligible, vlan_identifier, frame_check_sequence, service_access_point_identifier, connection_identifier, flow_hash, time_to_live) There's a big note in IEEE 802.1Q that says: The destination_address, source_address, mac_service_data_unit, priority, drop_eligible, service_access_point_identifier, connection_identifier, and frame_check_sequence parameters are as defined for the ISS. So basically, although the EISS extends the ISS, it has not changed the aspects of it regarding what constitutes a destination_address. So there is nothing that redefines the promiscuity concept to extend it with the vlan_identifier. Additionally, the 802.1Q spec talks about this EISS Multiplex Entity thing, which can be used by a VLAN-aware end station to provide a SAP (Service Access Point, in context it means an instance of the Internal Sublayer Service), one per VID of interest, to separate MAC clients. That is to say, the EISS Multiplex Entity provides multiple M_UNITDATA.indication and M_UNITDATA.request services to multiple MAC clients, one per VLAN. Each individual service can be in "promiscuous" mode. This is similar to how in Linux, each 8021q upper of a physical interface can be promiscuous or not. > > > > If I don't mark this change as a bug fix but as > > > > a simple patch, somebody could claim it's a regression, since promiscuity > > > > used to be enough to see packets with unknown VLANs, and now it no > > > > longer is... > > > > > > Can we take it into net-next? What's your feeling on that option? > > > > I see how you can view this patch as pointless, but there is some > > context to it. It isn't just for tcpdump/debugging, instead NXP has some > > TSN use cases which involve some asymmetric tc-vlan rules, which is how > > I arrived at this topic in the first place. I've already established > > that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off: > > https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/ > > Wasn't the conclusion that the VID should be added to the filter so it > also works with vlan filter enabled? Am I missing another discussion? Well, the conclusion was just that a tc-flower key that contains a VLAN ID will not be accepted by a VLAN-filtering NIC. Similarly, a tc-flower key that contains a destination MAC address will not be accepted by a NIC with IFF_UNICAST_FLT. There was no further discussion, it is just an elementary deduction from that point. There are two equally valid options: - make tc-flower use the vlan_vid_add API when it installs a vlan_id key, and the dev_uc_add/dev_mc_add API when it installs a dst_mac key OR - disable VLAN filtering if you're using vlan_id keys on VLAN-aware NICs, and put the interface in promiscuous mode if you're using dst_mac keys that are different from the NIC's filtering list. I chose option 2 because it was way simpler and was just as correct.
Am 2021-02-28 23:48, schrieb Vladimir Oltean: > On Sat, Feb 27, 2021 at 02:18:20PM +0100, Michael Walle wrote: >> Am 2021-02-27 01:16, schrieb Vladimir Oltean: >> > On Fri, Feb 26, 2021 at 03:49:22PM -0800, Jakub Kicinski wrote: >> > > On Sat, 27 Feb 2021 01:42:44 +0200 Vladimir Oltean wrote: >> > > > On Fri, Feb 26, 2021 at 03:28:36PM -0800, Jakub Kicinski wrote: >> > > > > I don't understand what you're fixing tho. >> > > > > >> > > > > Are we trying to establish vlan-filter-on as the expected behavior? >> > > > >> > > > What I'm fixing is unexpected behavior, according to the applicable >> > > > standards I could find. >> >> In the referenced thread you quoted from the IEEE802.3 about the >> promisc >> mode. >> The MAC sublayer may also provide the capability of operating in the >> promiscuous receive mode. In this mode of operation, the MAC >> sublayer >> recognizes and accepts all valid frames, regardless of their >> Destination >> Address field values. >> >> Your argument was that the standard just talks about disabling the >> DMAC >> filter. But was that really the _intention_ of the standard? Does the >> standard even mention a possible vlan tag? What I mean is: maybe the >> standard just mention the DMAC because it is the only filtering >> mechanism >> in this standard and it's enough to disable it to "accept all valid >> frames". >> >> I was biten by "the NIC drops frames with an unknown VLAN" even if >> promisc mode was enabled. And IMHO it was quite suprising for me. > > In short, promiscuity is a function of the MAC sublayer, which is the > lower portion of the Data Link Layer (the higher portion being the > Logical Link Control layer - LLC). The MAC sublayer is governed by IEEE > 802.3, and IEEE 802.1Q does not change anything related to promiscuity, > so everything still applies. > > The MAC sublayer provides its services to the MAC client through > something called the MAC service, which uses the following primitives: > > MA_DATA.request( > destination_address, > source_address, > mac_service_data_unit, > frame_check_sequence) > > to send a frame, and > > MA_DATA.indication( > destination_address, > source_address, > mac_service_data_unit, > frame_check_sequence, > ReceiveStatus) > > to receive a frame. > > One particular component of the MAC sublayer seems to be called the > Internal Sublayer Service (ISS), and this one is defined in IEEE > 802.1AC-2016. To be frank, I don't quite grok why there needs to exist > this extra layering, but nonetheless, the ISS has some similar service > primitives to the MAC sublayer as well, and these are: > > M_UNITDATA.indication( > destination_address, > source_address, > mac_service_data_unit, > priority, > drop_eligible, > frame_check_sequence, > service_access_point_identifier, > connection_identifier) > > M_UNITDATA.request( > destination_address, > source_address, > mac_service_data_unit, > priority, > drop_eligible, > frame_check_sequence, > service_access_point_identifier, > connection_identifier) > > where a "unit of data" is basically just very pompous speak for > "a frame", I guess. > > Promiscuity is defined in IEEE 802.3 clause 4A.2.9 Frame reception, > which _in_context_ talks about the interface between the MAC client and > the MAC sublayer, so that means about the M_UNITDATA.indication or the > MA_DATA.indication. > > Whereas VLAN filtering, as well as adding and removing VLAN tags, is > governed by IEEE 802.1Q, as a function of the Enhanced Internal > Sublayer > Service (EISS), i.e. clause 6.8. In fact, the EISS is just an ISS > enhanced for VLAN filtering, as the naming and definition implies. > > Of course (why not), the EISS has its own service primitives towards > its > higher-level clients for transmitting and receiving a frame. These are: > > EM_UNITDATA.request( > destination_address, > source_address, > mac_service_data_unit, > priority, > drop_eligible, > vlan_identifier, > frame_check_sequence, > service_access_point_identifier, > connection_identifier, > flow_hash, > time_to_live) > > EM_UNITDATA.indication( > destination_address, > source_address, > mac_service_data_unit, > priority, > drop_eligible, > vlan_identifier, > frame_check_sequence, > service_access_point_identifier, > connection_identifier, > flow_hash, > time_to_live) > > There's a big note in IEEE 802.1Q that says: > > The destination_address, source_address, mac_service_data_unit, > priority, drop_eligible, service_access_point_identifier, > connection_identifier, and frame_check_sequence parameters are as > defined for the ISS. > > So basically, although the EISS extends the ISS, it has not changed the > aspects of it regarding what constitutes a destination_address. So > there > is nothing that redefines the promiscuity concept to extend it with the > vlan_identifier. > > Additionally, the 802.1Q spec talks about this EISS Multiplex Entity > thing, which can be used by a VLAN-aware end station to provide a SAP > (Service Access Point, in context it means an instance of the Internal > Sublayer Service), one per VID of interest, to separate MAC clients. > That is to say, the EISS Multiplex Entity provides multiple > M_UNITDATA.indication and M_UNITDATA.request services to multiple MAC > clients, one per VLAN. Each individual service can be in "promiscuous" > mode. This is similar to how in Linux, each 8021q upper of a physical > interface can be promiscuous or not. Ok, I see, so your proposed behavior is backed by the standards. But OTOH there was a summary by Markus of the behavior of other drivers: https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/ And a conclusion by Jakub: https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t And a propsed core change to disable vlan filtering with promisc mode. Do I understand you correctly, that this shouldn't be done either? Don't get me wrong, I don't vote against or in favor of this patch. I just want to understand the behavior. I haven't had time to actually test this, but what if you do: - don't load the 8021q module (or don't enable kernel support) - enable promisc (1) - load 8021q module (2) - add a vlan interface (3) - add another vlan interface (4) What frames would you actually receive on the base interface in (1), (2), (3), (4) and what is the user expectation? I'd say its the same every time. (IIRC there is already some discrepancy due to the VLAN filter hardware offloading) >> > > > If I don't mark this change as a bug fix but as >> > > > a simple patch, somebody could claim it's a regression, since promiscuity >> > > > used to be enough to see packets with unknown VLANs, and now it no >> > > > longer is... >> > > >> > > Can we take it into net-next? What's your feeling on that option? >> > >> > I see how you can view this patch as pointless, but there is some >> > context to it. It isn't just for tcpdump/debugging, instead NXP has some >> > TSN use cases which involve some asymmetric tc-vlan rules, which is how >> > I arrived at this topic in the first place. I've already established >> > that tc-vlan only works with ethtool -K eth0 rx-vlan-filter off: >> > https://lore.kernel.org/netdev/CA+h21hoxwRdhq4y+w8Kwgm74d4cA0xLeiHTrmT-VpSaM7obhkg@mail.gmail.com/ >> >> Wasn't the conclusion that the VID should be added to the filter so it >> also works with vlan filter enabled? Am I missing another discussion? > > Well, the conclusion was just that a tc-flower key that contains a VLAN > ID will not be accepted by a VLAN-filtering NIC. Similarly, a tc-flower > key that contains a destination MAC address will not be accepted by a > NIC with IFF_UNICAST_FLT. > > There was no further discussion, it is just an elementary deduction > from > that point. There are two equally valid options: > - make tc-flower use the vlan_vid_add API when it installs a vlan_id > key, and the dev_uc_add/dev_mc_add API when it installs a dst_mac key > OR > - disable VLAN filtering if you're using vlan_id keys on VLAN-aware > NICs, and put the interface in promiscuous mode if you're using > dst_mac keys that are different from the NIC's filtering list. > > I chose option 2 because it was way simpler and was just as correct. Fair, but it will also put additional burden to the user to also disable the vlan filtering, right?. Otherwise it would just work. And it will waste CPU cycles for unwanted frames. Although your new patch version contains a new "(yet)" ;) -michael
On Mon, Mar 01, 2021 at 03:36:15PM +0100, Michael Walle wrote: > Ok, I see, so your proposed behavior is backed by the standards. But > OTOH there was a summary by Markus of the behavior of other drivers: > https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/ > And a conclusion by Jakub: > https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t > And a propsed core change to disable vlan filtering with promisc mode. > Do I understand you correctly, that this shouldn't be done either? > > Don't get me wrong, I don't vote against or in favor of this patch. > I just want to understand the behavior. So you can involuntarily ignore a standard, or you can ignore it deliberately. I can't force anyone to not ignore it in the latter case, but indeed, now that I tried to look it up, I personally don't think that promiscuity should disable VLAN filtering unless somebody comes up with a good reason for which Linux should basically disregard IEEE 802.3. In particular, Jakub seems to have been convinced in that thread by no other argument except that other drivers ignore the standards too, which I'm not sure is a convincing enough argument. In my opinion, the fact that some drivers disable VLAN filtering should be treated like a marginal condition, similar to how, when you set the MTU on an interface to N octets, it might happen that it accepts packets larger than N octets, but it isn't guaranteed. > I haven't had time to actually test this, but what if you do: > - don't load the 8021q module (or don't enable kernel support) > - enable promisc > (1) > - load 8021q module > (2) > - add a vlan interface > (3) > - add another vlan interface > (4) > > What frames would you actually receive on the base interface > in (1), (2), (3), (4) and what is the user expectation? > I'd say its the same every time. (IIRC there is already some > discrepancy due to the VLAN filter hardware offloading) The default value is: ethtool -k eno0 | grep rx-vlan-filter rx-vlan-filter: off so we receive all VLAN-tagged packets by default in enetc, unless VLAN filtering is turned on. > > I chose option 2 because it was way simpler and was just as correct. > > Fair, but it will also put additional burden to the user to also > disable the vlan filtering, right?. Otherwise it would just work. And > it will waste CPU cycles for unwanted frames. > Although your new patch version contains a new "(yet)" ;) True, nobody said it's optimal, but you can't make progress if you always try to do things optimally the first time (but at least you should do something that's not wrong). Adding the dev_uc_add, dev_mc_add and vlan_vid_add calls to net/sched/cls_flower.c doesn't seem an impossible task (especially since all of them are refcounted, it should be pretty simple to avoid strange interactions with other layers such as 8021q), but nonetheless, it just wasn't (and still isn't) high enough on my list of priorities.
On Mon, Mar 01, 2021 at 05:08:52PM +0200, Vladimir Oltean wrote: > On Mon, Mar 01, 2021 at 03:36:15PM +0100, Michael Walle wrote: > > Ok, I see, so your proposed behavior is backed by the standards. But > > OTOH there was a summary by Markus of the behavior of other drivers: > > https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/ > > And a conclusion by Jakub: > > https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t > > And a propsed core change to disable vlan filtering with promisc mode. > > Do I understand you correctly, that this shouldn't be done either? > > > > Don't get me wrong, I don't vote against or in favor of this patch. > > I just want to understand the behavior. > > So you can involuntarily ignore a standard, or you can ignore it > deliberately. I can't force anyone to not ignore it in the latter case, > but indeed, now that I tried to look it up, I personally don't think > that promiscuity should disable VLAN filtering unless somebody comes up > with a good reason for which Linux should basically disregard IEEE 802.3. > In particular, Jakub seems to have been convinced in that thread by no > other argument except that other drivers ignore the standards too, which > I'm not sure is a convincing enough argument. Admittedly, I am still not entirely convinced myself. I don't know why the other drivers do what they do, why they do it and whether that's correct. That is one of the reasons (next to quite a few issues I had with patching net/core) why I ungracefully abandoned the mentioned thread for now ( sorry and shame on me :-/ ). The main problem here could also just be that almost everybody _thinks_ that promiscuity means receiving all frames and no one is aware of the standards definition. In fact, I can't blame them, as the standard is hard to come by and not enjoyable to read, imho. And all secondary documentation I could find on the internet explain promiscuous mode as a "mode of operation" in which "the card accepts every Ethernet packet sent on the network" or similar. Even libpcap, which I consider the reference on network sniffing, thinks that "Promiscuous mode [...] sniffs all traffic on the wire." Thus I still think that this issue is also fixable by proper documentation of promiscuity. At least the meaning and guarantees of IFF_PROMISC in this kernel should be clearly defined - in one way or the other - such that users with different expectations can be directed there and drivers with different behavior can be fixed with that definition as justification. > > In my opinion, the fact that some drivers disable VLAN filtering should > be treated like a marginal condition, similar to how, when you set the > MTU on an interface to N octets, it might happen that it accepts packets > larger than N octets, but it isn't guaranteed. > > > I haven't had time to actually test this, but what if you do: > > - don't load the 8021q module (or don't enable kernel support) > > - enable promisc > > (1) > > - load 8021q module > > (2) > > - add a vlan interface > > (3) > > - add another vlan interface > > (4) > > > > What frames would you actually receive on the base interface > > in (1), (2), (3), (4) and what is the user expectation? > > I'd say its the same every time. (IIRC there is already some > > discrepancy due to the VLAN filter hardware offloading) > > The default value is: > ethtool -k eno0 | grep rx-vlan-filter > rx-vlan-filter: off > > so we receive all VLAN-tagged packets by default in enetc, unless VLAN > filtering is turned on. > > > > I chose option 2 because it was way simpler and was just as correct. > > > > Fair, but it will also put additional burden to the user to also > > disable the vlan filtering, right?. Otherwise it would just work. And > > it will waste CPU cycles for unwanted frames. > > Although your new patch version contains a new "(yet)" ;) > > True, nobody said it's optimal, but you can't make progress if you > always try to do things optimally the first time (but at least you > should do something that's not wrong). > Adding the dev_uc_add, dev_mc_add and vlan_vid_add calls to > net/sched/cls_flower.c doesn't seem an impossible task (especially since > all of them are refcounted, it should be pretty simple to avoid strange > interactions with other layers such as 8021q), but nonetheless, it just > wasn't (and still isn't) high enough on my list of priorities.
On Mon, Mar 01, 2021 at 05:26:53PM +0100, Markus Blöchl wrote: > The main problem here could also just be that almost everybody _thinks_ > that promiscuity means receiving all frames and no one is aware of the > standards definition. > In fact, I can't blame them, as the standard is hard to come by and not > enjoyable to read, imho. And all secondary documentation I could find > on the internet explain promiscuous mode as a "mode of operation" in which > "the card accepts every Ethernet packet sent on the network" or similar. > Even libpcap, which I consider the reference on network sniffing, thinks > that "Promiscuous mode [...] sniffs all traffic on the wire." > > Thus I still think that this issue is also fixable by proper > documentation of promiscuity. > At least the meaning and guarantees of IFF_PROMISC in this kernel should > be clearly defined - in one way or the other - such that users with > different expectations can be directed there and drivers with different > behavior can be fixed with that definition as justification. If Jakub and/or David give us the ACK, I will go ahead and update the documentation (probably Documentation/networking/netdevices.rst) to mention what does IFF_PROMISC cover, _separate_ from this series.
On Mon, 1 Mar 2021 19:02:51 +0200 Vladimir Oltean wrote: > On Mon, Mar 01, 2021 at 05:26:53PM +0100, Markus Blöchl wrote: > > The main problem here could also just be that almost everybody _thinks_ > > that promiscuity means receiving all frames and no one is aware of the > > standards definition. > > In fact, I can't blame them, as the standard is hard to come by and not > > enjoyable to read, imho. And all secondary documentation I could find > > on the internet explain promiscuous mode as a "mode of operation" in which > > "the card accepts every Ethernet packet sent on the network" or similar. > > Even libpcap, which I consider the reference on network sniffing, thinks > > that "Promiscuous mode [...] sniffs all traffic on the wire." > > > > Thus I still think that this issue is also fixable by proper > > documentation of promiscuity. > > At least the meaning and guarantees of IFF_PROMISC in this kernel should > > be clearly defined - in one way or the other - such that users with > > different expectations can be directed there and drivers with different > > behavior can be fixed with that definition as justification. > > If Jakub and/or David give us the ACK, I will go ahead and update the > documentation (probably Documentation/networking/netdevices.rst) to > mention what does IFF_PROMISC cover, _separate_ from this series. How do we do this in practical terms? It'd definitely be very useful to write up the discussions but we can't expect that all existing drivers get converted, and incorrect documentation is worse than none at all. IIRC Ido also pointed out that the bridge driver follows the "promisc includes VLAN", so SW drivers would need to be updated as well. I personally agree with the interpretation that PROMISC == disable DA filter, but we can't reasonably expect all drivers to follow it. All I can think of is documenting that the driver _may_ not disable VLAN filter, and that's our recommendation for new drivers, therefore users who want "vlan promisc" _must_ disable rx-vlan but the inverse is not guaranteed (rx-vlan on + PROMISC == only frames for known VLANs). What's your thinking?
From: Vladimir Oltean <vladimir.oltean@nxp.com> This contains an assorted set of fixes collected over the past week on the enetc driver. Some are related to VLAN processing, some to physical link settings, some are fixups of previous hardware workarounds. Vladimir Oltean (6): net: enetc: don't overwrite the RSS indirection table when initializing net: enetc: initialize RFS/RSS memories for unused ports too net: enetc: take the MDIO lock only once per NAPI poll cycle net: enetc: fix incorrect TPID when receiving 802.1ad tagged packets net: enetc: don't disable VLAN filtering in IFF_PROMISC mode net: enetc: force the RGMII speed and duplex instead of operating in inband mode drivers/net/ethernet/freescale/enetc/enetc.c | 130 +++++------------- drivers/net/ethernet/freescale/enetc/enetc.h | 5 + .../net/ethernet/freescale/enetc/enetc_cbdr.c | 54 ++++++++ .../net/ethernet/freescale/enetc/enetc_hw.h | 18 ++- .../net/ethernet/freescale/enetc/enetc_pf.c | 103 +++++++++++--- .../net/ethernet/freescale/enetc/enetc_vf.c | 7 + 6 files changed, 203 insertions(+), 114 deletions(-)