: [VLAN] Turn off the HW vlan filter if promisc
Subsystems (2)
PATCH 4/5 : [VLAN] Turn off the HW vlan filter if promisc
If the netdev goes into mode promiscuous, receive all of the packets on the wire without HW filtering, those packets will be processed as type PACKET_OTHERHOST.
--- drivers/net/e1000/e1000_main.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)
drivers/net/e1000/e1000_main.c +14 -5
@@ -2495,7 +2495,7 @@ e1000_set_rx_mode(struct net_device *netdev)struct e1000_hw *hw = &adapter->hw;struct dev_addr_list *uc_ptr;struct dev_addr_list *mc_ptr; -uint32_t rctl; +uint32_t rctl, ctrl;uint32_t hash_value;int i, rar_entries =E1000_RAR_ENTRIES ; int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?
@@ -2512,13 +2512,19 @@ e1000_set_rx_mode(struct net_device *netdev)/* Check for Promiscuous and All Multicast modes */ rctl =E1000_READ_REG (hw,RCTL ); + ctrl =E1000_READ_REG (hw,CTRL );if (netdev->flags &IFF_PROMISC ) { rctl |= (E1000_RCTL_UPE |E1000_RCTL_MPE ); - }else if (netdev->flags &IFF_ALLMULTI ) { - rctl |=E1000_RCTL_MPE ; + rctl &= ~E1000_RCTL_VFE ; }else { - rctl &= ~E1000_RCTL_MPE ; +if (ctrl &E1000_CTRL_VME && + adapter->hw.mac_type != e1000_ich8lan) + rctl |=E1000_RCTL_VFE ; +if (netdev->flags &IFF_ALLMULTI ) + rctl |=E1000_RCTL_MPE ; +else + rctl &= ~E1000_RCTL_MPE ; } uc_ptr =NULL ;
@@ -5013,7 +5019,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)if (adapter->hw.mac_type != e1000_ich8lan) {/* enable VLAN receive filtering */ rctl =E1000_READ_REG (&adapter->hw,RCTL ); - rctl |=E1000_RCTL_VFE ; +if (netdev->flags &IFF_PROMISC ) + rctl &= ~E1000_RCTL_VFE ; +else + rctl |=E1000_RCTL_VFE ; rctl &= ~E1000_RCTL_CFIEN ;E1000_WRITE_REG (&adapter->hw,RCTL , rctl);e1000_update_mng_vlan (adapter);
Signature · 1 line
1.5.4.3
Quoted 5 lines · Joonwoo Park
If the netdev goes into mode promiscuous, receive all of the packets on the wire without HW filtering, those packets will be processed as type
PACKET_OTHERHOST.Signed-off-by Joonwoo Park <[email protected]>
this is a repost afaics with the same idea as previous.
I however can't make sense out of what the consensus was in the discussion that previously took place.
My techical objection against this patch is that it removes flexibility from the admin to display only the particular vlan traffic. With this patch there is no longer a way to sniff just one vlan and forget about the other traffic.
My logistical objection against this patch is that it adjusts only one/two drivers to do something, but I don't see anything consistently in all the drivers at all that suggest that this is the right thing to do at all. Either a mandate or something suggesting that all drivers that do vlan filtering *should* do something
specific in promiscuous mode.
Either it should be clear that all drivers should do the same thing (and you should then adjust all the drivers and not just the one or two that you use, and preferably in the stack if possible) or we leave this as is for all the drivers.
I think we should prevent wild growth like e.g. module parameters like vlan_tag_strip in s2io (NO_STRIP_IN_PROMISC)... !
so I don't think this is a good idea.
Auke
Quoted 52 lines
--- drivers/net/e1000/e1000_main.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)drivers/net/e1000/e1000_main.c +14
-5@@ -2495,7 +2495,7 @@ e1000_set_rx_mode(struct net_device *netdev)struct e1000_hw *hw = &adapter->hw;struct dev_addr_list *uc_ptr;struct dev_addr_list *mc_ptr; -uint32_t rctl; +uint32_t rctl, ctrl;uint32_t hash_value;int i, rar_entries =E1000_RAR_ENTRIES ; int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?@@ -2512,13 +2512,19 @@ e1000_set_rx_mode(struct net_device *netdev)/* Check for Promiscuous and All Multicast modes */ rctl =E1000_READ_REG (hw,RCTL ); + ctrl =E1000_READ_REG (hw,CTRL );if (netdev->flags &IFF_PROMISC ) { rctl |= (E1000_RCTL_UPE |E1000_RCTL_MPE ); - }else if (netdev->flags &IFF_ALLMULTI ) { - rctl |=E1000_RCTL_MPE ; + rctl &= ~E1000_RCTL_VFE ; }else { - rctl &= ~E1000_RCTL_MPE ; +if (ctrl &E1000_CTRL_VME && + adapter->hw.mac_type != e1000_ich8lan) + rctl |=E1000_RCTL_VFE ; +if (netdev->flags &IFF_ALLMULTI ) + rctl |=E1000_RCTL_MPE ; +else + rctl &= ~E1000_RCTL_MPE ; } uc_ptr =NULL ;@@ -5013,7 +5019,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)if (adapter->hw.mac_type != e1000_ich8lan) {/* enable VLAN receive filtering */ rctl =E1000_READ_REG (&adapter->hw,RCTL ); - rctl |=E1000_RCTL_VFE ; +if (netdev->flags &IFF_PROMISC ) + rctl &= ~E1000_RCTL_VFE ; +else + rctl |=E1000_RCTL_VFE ; rctl &= ~E1000_RCTL_CFIEN ;E1000_WRITE_REG (&adapter->hw,RCTL , rctl);e1000_update_mng_vlan (adapter);
2008/4/18, Kok, Auke <[email protected]>:
Quoted 5 lines
Quoted 4 lines · Joonwoo Park
If the netdev goes into mode promiscuous, receive all of the packets on the wire without HW filtering, those packets will be processed as type
PACKET_OTHERHOST.
Quoted 7 lines
Quoted 1 line
Signed-off-by Joonwoo Park <[email protected]>this is a repost afaics with the same idea as previous.
I however can't make sense out of what the consensus was in the discussion that previously took place.
Auke,
Thanks for your response
Please take a look at
http://thread.gmane.org/gmane.linux.network/88488
I agreed with Patrick's opinion.
Quoted 4 lines
My techical objection against this patch is that it removes flexibility from the admin to display only the particular vlan traffic. With this patch there is no longer a way to sniff just one vlan and forget about the other traffic.
No, the admin configured promisc flag on the netdev. It means that the admin would like to see all the packets on the wire not one particular vlan traffic I think.
If admin would like to sniff one vlan, he or she should configure promisc. This patch dose nothing for non-promisc.
Quoted 6 lines
My logistical objection against this patch is that it adjusts only one/two drivers to do something, but I don't see anything consistently in all the drivers at all that suggest that this is the right thing to do at all. Either a mandate or something suggesting that all drivers that do vlan filtering *should* do something
specific in promiscuous mode.
It's true. We need to fix all the netdev which is VLAN_HW_FILTER capable IMHO. However I couldn't make patches for all of them since I don't have those HW :(
Quoted 4 lines
Either it should be clear that all drivers should do the same thing (and you should then adjust all the drivers and not just the one or two that you use, and preferably in the stack if possible) or we leave this as is for all the drivers.
I'll try to adjust all the drivers or push it to other driver maintainers if people does like this patchset.
Quoted 3 lines
I think we should prevent wild growth like e.g. module parameters like
vlan_tag_stripin s2io (NO_STRIP_IN_PROMISC)... !
I don't think so :), we don't need NO_STRIP if I worked correctly. The patches 1,2 is the work for passing vlan tag to sniffers without no stripping.
Thanks,
Joonwoo
Quoted 60 lines
so I don't think this is a good idea.
Auke
Quoted 52 lines
--- drivers/net/e1000/e1000_main.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)drivers/net/e1000/e1000_main.c +14
-5@@ -2495,7 +2495,7 @@ e1000_set_rx_mode(struct net_device *netdev)struct e1000_hw *hw = &adapter->hw;struct dev_addr_list *uc_ptr;struct dev_addr_list *mc_ptr; -uint32_t rctl; +uint32_t rctl, ctrl;uint32_t hash_value;int i, rar_entries =E1000_RAR_ENTRIES ; int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?@@ -2512,13 +2512,19 @@ e1000_set_rx_mode(struct net_device *netdev)/* Check for Promiscuous and All Multicast modes */ rctl =E1000_READ_REG (hw,RCTL ); + ctrl =E1000_READ_REG (hw,CTRL );if (netdev->flags &IFF_PROMISC ) { rctl |= (E1000_RCTL_UPE |E1000_RCTL_MPE ); - }else if (netdev->flags &IFF_ALLMULTI ) { - rctl |=E1000_RCTL_MPE ; + rctl &= ~E1000_RCTL_VFE ; }else { - rctl &= ~E1000_RCTL_MPE ; +if (ctrl &E1000_CTRL_VME && + adapter->hw.mac_type != e1000_ich8lan) + rctl |=E1000_RCTL_VFE ; +if (netdev->flags &IFF_ALLMULTI ) + rctl |=E1000_RCTL_MPE ; +else + rctl &= ~E1000_RCTL_MPE ; } uc_ptr =NULL ;@@ -5013,7 +5019,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)if (adapter->hw.mac_type != e1000_ich8lan) {/* enable VLAN receive filtering */ rctl =E1000_READ_REG (&adapter->hw,RCTL ); - rctl |=E1000_RCTL_VFE ; +if (netdev->flags &IFF_PROMISC ) + rctl &= ~E1000_RCTL_VFE ; +else + rctl |=E1000_RCTL_VFE ; rctl &= ~E1000_RCTL_CFIEN ;E1000_WRITE_REG (&adapter->hw,RCTL , rctl);e1000_update_mng_vlan (adapter);
2008/4/18, Joonwoo Park <[email protected]>:
Quoted 4 lines
No, the admin configured promisc flag on the netdev. It means that the admin would like to see all the packets on the wire not one particular vlan traffic I think.
If admin would like to sniff one vlan, he or she should configure promisc.
sorry
^ not
Joonwoo
Quoted 9 lines · Joonwoo Park
2008/4/18, Kok, Auke <[email protected]>:
Quoted 5 lines
My logistical objection against this patch is that it adjusts only one/two drivers to do something, but I don't see anything consistently in all the drivers at all that suggest that this is the right thing to do at all. Either a mandate or something suggesting that all drivers that do vlan filtering *should* do something
specific in promiscuous mode.It's true. We need to fix all the netdev which is
VLAN_HW_FILTERcapable IMHO. However I couldn't make patches for all of them since I don't have those HW :(
sigh, we really should not make e1000(e) guinea pigs - especially at this time.
is there someone who can actually test this for us? I'd be a lot happier if I had some confidence that people who actually do use promisc/vlans give me a thumbs up.
Auke