[Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Jesse Brandeburg]

There was a problem reported to us where the addition of a VF with an IPv6
address ending with a particular sequence would cause the parent device on
the PF to no longer be able to respond to neighbor discovery packets.

In this case, we had an ovs-bridge device living on top of a VLAN, which
was on top of a PF, and it would not be able to talk anymore (the neighbor
entry would expire and couldn't be restored).

The root cause of the issue is that if the PF is asked to be in IFF_PROMISC
mode (promiscuous mode) and it had an ipv6 address that needed the
33:33:ff:00:00:04 multicast address to work, then when the VF was added
with the need for the same multicast address, the VF would steal all the
traffic destined for that address.

The ice driver didn't auto-subscribe a request of IFF_PROMISC to the
"multicast replication from other port's traffic" meaning that it won't get
for instance, packets with an exact destination in the VF, as above.

The VF's IPv6 address, which adds a "perfect filter" for 33:33:ff:00:00:04,
results in no packets for that multicast address making it to the PF (which
is in promisc but NOT "multicast replication").

The fix is to enable "multicast promiscuous" whenever the driver is asked
to enable IFF_PROMISC, and make sure to disable it when appropriate.

Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
v2: added fixes
---
 drivers/net/ethernet/intel/ice/ice_main.c | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 5f86e4111fa9..3a5f9c15b69c 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -275,6 +275,8 @@ static int ice_set_promisc(struct ice_vsi *vsi, u8 promisc_m)
 	if (status && status != -EEXIST)
 		return status;
 
+	netdev_dbg(vsi->netdev, "set promisc filter bits for VSI %i: 0x%x\n",
+		   vsi->vsi_num, promisc_m);
 	return 0;
 }
 
@@ -300,6 +302,8 @@ static int ice_clear_promisc(struct ice_vsi *vsi, u8 promisc_m)
 						    promisc_m, 0);
 	}
 
+	netdev_dbg(vsi->netdev, "clear promisc filter bits for VSI %i: 0x%x\n",
+		   vsi->vsi_num, promisc_m);
 	return status;
 }
 
@@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
 				}
 				err = 0;
 				vlan_ops->dis_rx_filtering(vsi);
+
+				/* promiscuous mode implies allmulticast so
+				 * that VSIs that are in promiscuous mode are
+				 * subscribed to multicast packets coming to
+				 * the port
+				 */
+				err = ice_set_promisc(vsi,
+						      ICE_MCAST_PROMISC_BITS);
+				if (err)
+					goto out_promisc;
 			}
 		} else {
 			/* Clear Rx filter to remove traffic from wire */
@@ -430,6 +444,18 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
 				    NETIF_F_HW_VLAN_CTAG_FILTER)
 					vlan_ops->ena_rx_filtering(vsi);
 			}
+
+			/* disable allmulti here, but only if allmulti is not
+			 * still enabled for the netdev
+			 */
+			if (!(vsi->current_netdev_flags & IFF_ALLMULTI)) {
+				err = ice_clear_promisc(vsi,
+							ICE_MCAST_PROMISC_BITS);
+				if (err) {
+					netdev_err(netdev, "Error %d clearing multicast promiscuous on VSI %i\n",
+						   err, vsi->vsi_num);
+				}
+			}
 		}
 	}
 	goto exit;

base-commit: 811d581194f7412eda97acc03d17fc77824b561f
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Paul Menzel]

Dear Jesse,


Thank you for your patch.

Am 07.02.23 um 00:54 schrieb Jesse Brandeburg:
> There was a problem reported to us where the addition of a VF with an IPv6
> address ending with a particular sequence would cause the parent device on
> the PF to no longer be able to respond to neighbor discovery packets.
> 
> In this case, we had an ovs-bridge device living on top of a VLAN, which
> was on top of a PF, and it would not be able to talk anymore (the neighbor
> entry would expire and couldn't be restored).
> 
> The root cause of the issue is that if the PF is asked to be in IFF_PROMISC
> mode (promiscuous mode) and it had an ipv6 address that needed the
> 33:33:ff:00:00:04 multicast address to work, then when the VF was added
> with the need for the same multicast address, the VF would steal all the
> traffic destined for that address.
> 
> The ice driver didn't auto-subscribe a request of IFF_PROMISC to the
> "multicast replication from other port's traffic" meaning that it won't get
> for instance, packets with an exact destination in the VF, as above.
> 
> The VF's IPv6 address, which adds a "perfect filter" for 33:33:ff:00:00:04,
> results in no packets for that multicast address making it to the PF (which
> is in promisc but NOT "multicast replication").
> 
> The fix is to enable "multicast promiscuous" whenever the driver is asked
> to enable IFF_PROMISC, and make sure to disable it when appropriate.

Do you have the commands to easily reproduce this?

Maybe also mention the new debug messages?

> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: added fixes
> ---
>   drivers/net/ethernet/intel/ice/ice_main.c | 26 +++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 5f86e4111fa9..3a5f9c15b69c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -275,6 +275,8 @@ static int ice_set_promisc(struct ice_vsi *vsi, u8 promisc_m)
>   	if (status && status != -EEXIST)
>   		return status;
>   
> +	netdev_dbg(vsi->netdev, "set promisc filter bits for VSI %i: 0x%x\n",
> +		   vsi->vsi_num, promisc_m);
>   	return 0;
>   }
>   
> @@ -300,6 +302,8 @@ static int ice_clear_promisc(struct ice_vsi *vsi, u8 promisc_m)
>   						    promisc_m, 0);
>   	}
>   
> +	netdev_dbg(vsi->netdev, "clear promisc filter bits for VSI %i: 0x%x\n",
> +		   vsi->vsi_num, promisc_m);
>   	return status;
>   }
>   
> @@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>   				}
>   				err = 0;
>   				vlan_ops->dis_rx_filtering(vsi);
> +
> +				/* promiscuous mode implies allmulticast so
> +				 * that VSIs that are in promiscuous mode are
> +				 * subscribed to multicast packets coming to
> +				 * the port
> +				 */
> +				err = ice_set_promisc(vsi,
> +						      ICE_MCAST_PROMISC_BITS);
> +				if (err)
> +					goto out_promisc;
>   			}
>   		} else {
>   			/* Clear Rx filter to remove traffic from wire */
> @@ -430,6 +444,18 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>   				    NETIF_F_HW_VLAN_CTAG_FILTER)
>   					vlan_ops->ena_rx_filtering(vsi);
>   			}
> +
> +			/* disable allmulti here, but only if allmulti is not
> +			 * still enabled for the netdev

Above you write *allmulticast*. Should you use it here for consistency?

> +			 */
> +			if (!(vsi->current_netdev_flags & IFF_ALLMULTI)) {
> +				err = ice_clear_promisc(vsi,
> +							ICE_MCAST_PROMISC_BITS);
> +				if (err) {
> +					netdev_err(netdev, "Error %d clearing multicast promiscuous on VSI %i\n",
> +						   err, vsi->vsi_num);
> +				}
> +			}
>   		}
>   	}
>   	goto exit;


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Jesse Brandeburg]

On 2/6/2023 10:39 PM, Paul Menzel wrote:
> Dear Jesse,
> 
> 
> Thank you for your patch.
> 
> Am 07.02.23 um 00:54 schrieb Jesse Brandeburg:
>> There was a problem reported to us where the addition of a VF with an 
>> IPv6
>> address ending with a particular sequence would cause the parent 
>> device on
>> the PF to no longer be able to respond to neighbor discovery packets.
>>
>> In this case, we had an ovs-bridge device living on top of a VLAN, which
>> was on top of a PF, and it would not be able to talk anymore (the 
>> neighbor
>> entry would expire and couldn't be restored).
>>
>> The root cause of the issue is that if the PF is asked to be in 
>> IFF_PROMISC
>> mode (promiscuous mode) and it had an ipv6 address that needed the
>> 33:33:ff:00:00:04 multicast address to work, then when the VF was added
>> with the need for the same multicast address, the VF would steal all the
>> traffic destined for that address.
>>
>> The ice driver didn't auto-subscribe a request of IFF_PROMISC to the
>> "multicast replication from other port's traffic" meaning that it 
>> won't get
>> for instance, packets with an exact destination in the VF, as above.
>>
>> The VF's IPv6 address, which adds a "perfect filter" for 
>> 33:33:ff:00:00:04,
>> results in no packets for that multicast address making it to the PF 
>> (which
>> is in promisc but NOT "multicast replication").
>>
>> The fix is to enable "multicast promiscuous" whenever the driver is asked
>> to enable IFF_PROMISC, and make sure to disable it when appropriate.
> 
> Do you have the commands to easily reproduce this?

We do, but they're quite involved. Here is my reproducer script.
#!/bin/bash

set -x
#set -e
set -o pipefail

#### system A setup
DEVICE=ens259f0
ip link set $DEVICE up
ip link add link $DEVICE name $DEVICE.3 type vlan id 3
ip link set $DEVICE.3 up
echo 0 > /proc/sys/net/ipv6/conf/$DEVICE/accept_ra
echo 0 > /proc/sys/net/ipv6/conf/$DEVICE.3/accept_ra
ip addr flush $DEVICE
ip addr flush $DEVICE.3

# using linux bridge doesn't reproduce this bug
#ip link add name br0 type bridge
#ip link set $DEVICE.3 master br0
#echo 0 > /proc/sys/net/ipv6/conf/br0/accept_ra
#ip addr flush br0
#ip addr add 2001:1b74:4d9:1002::4/64 dev br0
#ip link set br0 up

# use openvswitch to reproduce this bug
systemctl start openvswitch
ovs-vsctl add-br ovs-br0 || echo keep going!
ovs-vsctl add-port ovs-br0 $DEVICE.3|| echo keep going!
echo 0 > /proc/sys/net/ipv6/conf/ovs-br0/accept_ra
ip addr flush ovs-br0
ip addr add 2001:1b74:4d9:1002::4/64 dev ovs-br0
ip link set ovs-br0 up

#### system B setup (coyote30)
###  make sure to set up passwordless ssh B->A
RDEVICE=ens259f0
RHOST=coyote30
ssh $RHOST sysctl -e net.ipv6.conf.$RDEVICE.accept_ra=0
ssh $RHOST ip link add link $RDEVICE name $RDEVICE.3 type vlan id 3
ssh $RHOST sysctl -e net.ipv6.conf.$RDEVICE.3.accept_ra=0
ssh $RHOST ip addr add 2001:1b74:4d9:1002::5/64 dev $RDEVICE.3
ssh $RHOST ip link set $RDEVICE.3 up

ssh $RHOST ping6 -c2 2001:1b74:4d9:1002::4

#### back to system A
echo 0 > /sys/class/net/$DEVICE/device/sriov_numvfs
echo 8 > /sys/class/net/$DEVICE/device/sriov_numvfs
sleep 5
ip link set $DEVICE vf 7 vlan 4
ip link set $DEVICE"v7" up
echo 0 > /proc/sys/net/ipv6/conf/$DEVICE"v7"/accept_ra
sleep 2
ip addr flush $DEVICE"v7"
sleep 2
ip addr add 2001:1b74:4d9:1105::4/64 dev $DEVICE"v7"
ip l set $DEVICE"v7" up
sleep 5

#### back to system B
ssh $RHOST ip neighbour del 2001:1b74:4d9:1002::4 dev $RDEVICE.3
# the below command is expected to fail if the bug is present
ssh $RHOST ping6 -c2 2001:1b74:4d9:1002::4

#### back to system A
# executing the below command will bring back the ping
#ip addr flush $DEVICE"v7"




> Maybe also mention the new debug messages?

I don't think this is worth resending the patch unless Tony disagrees.

Thanks for the review!


> 
>> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and 
>> bump version")
>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> ---
>> v2: added fixes
>> ---
>>   drivers/net/ethernet/intel/ice/ice_main.c | 26 +++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c 
>> b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 5f86e4111fa9..3a5f9c15b69c 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -275,6 +275,8 @@ static int ice_set_promisc(struct ice_vsi *vsi, u8 
>> promisc_m)
>>       if (status && status != -EEXIST)
>>           return status;
>> +    netdev_dbg(vsi->netdev, "set promisc filter bits for VSI %i: 
>> 0x%x\n",
>> +           vsi->vsi_num, promisc_m);
>>       return 0;
>>   }
>> @@ -300,6 +302,8 @@ static int ice_clear_promisc(struct ice_vsi *vsi, 
>> u8 promisc_m)
>>                               promisc_m, 0);
>>       }
>> +    netdev_dbg(vsi->netdev, "clear promisc filter bits for VSI %i: 
>> 0x%x\n",
>> +           vsi->vsi_num, promisc_m);
>>       return status;
>>   }
>> @@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>>                   }
>>                   err = 0;
>>                   vlan_ops->dis_rx_filtering(vsi);
>> +
>> +                /* promiscuous mode implies allmulticast so
>> +                 * that VSIs that are in promiscuous mode are
>> +                 * subscribed to multicast packets coming to
>> +                 * the port
>> +                 */
>> +                err = ice_set_promisc(vsi,
>> +                              ICE_MCAST_PROMISC_BITS);
>> +                if (err)
>> +                    goto out_promisc;
>>               }
>>           } else {
>>               /* Clear Rx filter to remove traffic from wire */
>> @@ -430,6 +444,18 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>>                       NETIF_F_HW_VLAN_CTAG_FILTER)
>>                       vlan_ops->ena_rx_filtering(vsi);
>>               }
>> +
>> +            /* disable allmulti here, but only if allmulti is not
>> +             * still enabled for the netdev
> 
> Above you write *allmulticast*. Should you use it here for consistency?

This location is actually checking the allmulti flag, so I think it's ok.

> 
>> +             */
>> +            if (!(vsi->current_netdev_flags & IFF_ALLMULTI)) {
>> +                err = ice_clear_promisc(vsi,
>> +                            ICE_MCAST_PROMISC_BITS);
>> +                if (err) {
>> +                    netdev_err(netdev, "Error %d clearing multicast 
>> promiscuous on VSI %i\n",
>> +                           err, vsi->vsi_num);
>> +                }
>> +            }
>>           }
>>       }
>>       goto exit;
> 
> 
> Kind regards,
> 
> Paul

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Tony Nguyen]

On 2/7/2023 11:05 AM, Jesse Brandeburg wrote:
> On 2/6/2023 10:39 PM, Paul Menzel wrote:

...

>> Maybe also mention the new debug messages?
> 
> I don't think this is worth resending the patch unless Tony disagrees.
>

It wouldn't of hurt but, agree, probably not worth another version just 
for this.

Thanks,
Tony
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Michal Swiatkowski]

On Mon, Feb 06, 2023 at 03:54:36PM -0800, Jesse Brandeburg wrote:
> There was a problem reported to us where the addition of a VF with an IPv6
> address ending with a particular sequence would cause the parent device on
> the PF to no longer be able to respond to neighbor discovery packets.
> 
> In this case, we had an ovs-bridge device living on top of a VLAN, which
> was on top of a PF, and it would not be able to talk anymore (the neighbor
> entry would expire and couldn't be restored).
> 
> The root cause of the issue is that if the PF is asked to be in IFF_PROMISC
> mode (promiscuous mode) and it had an ipv6 address that needed the
> 33:33:ff:00:00:04 multicast address to work, then when the VF was added
> with the need for the same multicast address, the VF would steal all the
> traffic destined for that address.
> 
> The ice driver didn't auto-subscribe a request of IFF_PROMISC to the
> "multicast replication from other port's traffic" meaning that it won't get
> for instance, packets with an exact destination in the VF, as above.
> 
> The VF's IPv6 address, which adds a "perfect filter" for 33:33:ff:00:00:04,
> results in no packets for that multicast address making it to the PF (which
> is in promisc but NOT "multicast replication").
> 
> The fix is to enable "multicast promiscuous" whenever the driver is asked
> to enable IFF_PROMISC, and make sure to disable it when appropriate.
> 
> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: added fixes
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 26 +++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 5f86e4111fa9..3a5f9c15b69c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -275,6 +275,8 @@ static int ice_set_promisc(struct ice_vsi *vsi, u8 promisc_m)
>  	if (status && status != -EEXIST)
>  		return status;
>  
> +	netdev_dbg(vsi->netdev, "set promisc filter bits for VSI %i: 0x%x\n",
> +		   vsi->vsi_num, promisc_m);
>  	return 0;
>  }
>  
> @@ -300,6 +302,8 @@ static int ice_clear_promisc(struct ice_vsi *vsi, u8 promisc_m)
>  						    promisc_m, 0);
>  	}
>  
> +	netdev_dbg(vsi->netdev, "clear promisc filter bits for VSI %i: 0x%x\n",
> +		   vsi->vsi_num, promisc_m);
>  	return status;
>  }
>  
> @@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>  				}
>  				err = 0;
>  				vlan_ops->dis_rx_filtering(vsi);
> +
> +				/* promiscuous mode implies allmulticast so
> +				 * that VSIs that are in promiscuous mode are
> +				 * subscribed to multicast packets coming to
> +				 * the port
> +				 */
> +				err = ice_set_promisc(vsi,
> +						      ICE_MCAST_PROMISC_BITS);
> +				if (err)
> +					goto out_promisc;
Aren't we already doing the same thing in case of IFF_ALLMULTI?
I wonder if our IFF_PROMISC handling is correct. Currently IFF_PROMISC
means setting PF VSI as default -> all packets from switch that don't
match any active rule goes to this VSI. If there is a rule (like in case
from the commit message) packet doesn't go to this VSI. Maybe it should
replicate all packets to PF VSI even there is a matching rule?

Basically, how IFF_PROMISC should work?

Thanks, Michal
>  			}
>  		} else {
>  			/* Clear Rx filter to remove traffic from wire */
> @@ -430,6 +444,18 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>  				    NETIF_F_HW_VLAN_CTAG_FILTER)
>  					vlan_ops->ena_rx_filtering(vsi);
>  			}
> +
> +			/* disable allmulti here, but only if allmulti is not
> +			 * still enabled for the netdev
> +			 */
> +			if (!(vsi->current_netdev_flags & IFF_ALLMULTI)) {
> +				err = ice_clear_promisc(vsi,
> +							ICE_MCAST_PROMISC_BITS);
> +				if (err) {
> +					netdev_err(netdev, "Error %d clearing multicast promiscuous on VSI %i\n",
> +						   err, vsi->vsi_num);
> +				}
> +			}
>  		}
>  	}
>  	goto exit;
> 
> base-commit: 811d581194f7412eda97acc03d17fc77824b561f
> -- 
> 2.31.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Jesse Brandeburg]

On 2/8/2023 5:34 AM, Michal Swiatkowski wrote:
>> @@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>>   				}
>>   				err = 0;
>>   				vlan_ops->dis_rx_filtering(vsi);
>> +
>> +				/* promiscuous mode implies allmulticast so
>> +				 * that VSIs that are in promiscuous mode are
>> +				 * subscribed to multicast packets coming to
>> +				 * the port
>> +				 */
>> +				err = ice_set_promisc(vsi,
>> +						      ICE_MCAST_PROMISC_BITS);
>> +				if (err)
>> +					goto out_promisc;
> Aren't we already doing the same thing in case of IFF_ALLMULTI?
> I wonder if our IFF_PROMISC handling is correct. Currently IFF_PROMISC
> means setting PF VSI as default -> all packets from switch that don't
                                                                   ^^^^^
that's the critical failure of the original code.

> match any active rule goes to this VSI. If there is a rule (like in case
> from the commit message) packet doesn't go to this VSI. Maybe it should
> replicate all packets to PF VSI even there is a matching rule?

That's what this code change does, turn on the MCAST_PROMISC_BITS flag 
which makes the hardware replicate all multicasts received with *other* 
destination VSIs to this PF one.

> 
> Basically, how IFF_PROMISC should work?

Yep, it's always how it should have worked, but it wasn't quite working 
right, and using bridge "fixed" it because bridge sets both PROMISC and 
ALLMULTI bits.


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Samudrala, Sridhar]

On 2/9/2023 8:08 PM, Jesse Brandeburg wrote:
> On 2/8/2023 5:34 AM, Michal Swiatkowski wrote:
>>> @@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>>>                   }
>>>                   err = 0;
>>>                   vlan_ops->dis_rx_filtering(vsi);
>>> +
>>> +                /* promiscuous mode implies allmulticast so
>>> +                 * that VSIs that are in promiscuous mode are
>>> +                 * subscribed to multicast packets coming to
>>> +                 * the port
>>> +                 */
>>> +                err = ice_set_promisc(vsi,
>>> +                              ICE_MCAST_PROMISC_BITS);
>>> +                if (err)
>>> +                    goto out_promisc;
>> Aren't we already doing the same thing in case of IFF_ALLMULTI?
>> I wonder if our IFF_PROMISC handling is correct. Currently IFF_PROMISC
>> means setting PF VSI as default -> all packets from switch that don't
>                                                                    ^^^^^
> that's the critical failure of the original code.
> 
>> match any active rule goes to this VSI. If there is a rule (like in case
>> from the commit message) packet doesn't go to this VSI. Maybe it should
>> replicate all packets to PF VSI even there is a matching rule?
> 

> That's what this code change does, turn on the MCAST_PROMISC_BITS flag 
> which makes the hardware replicate all multicasts received with *other* 
> destination VSIs to this PF one.
> 
>>
>> Basically, how IFF_PROMISC should work?
> 
> Yep, it's always how it should have worked, but it wasn't quite working 
> right, and using bridge "fixed" it because bridge sets both PROMISC and 
> ALLMULTI bits.

Can this be solved by letting ovs set multicast snooping rather than 
driver overloading IFF_PROMISC
    ovs-vsctl set Bridge br0 mcast_snooping_enable=true


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Michal Swiatkowski]

On Thu, Feb 09, 2023 at 06:08:45PM -0800, Jesse Brandeburg wrote:
> On 2/8/2023 5:34 AM, Michal Swiatkowski wrote:
> > > @@ -414,6 +418,16 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
> > >   				}
> > >   				err = 0;
> > >   				vlan_ops->dis_rx_filtering(vsi);
> > > +
> > > +				/* promiscuous mode implies allmulticast so
> > > +				 * that VSIs that are in promiscuous mode are
> > > +				 * subscribed to multicast packets coming to
> > > +				 * the port
> > > +				 */
> > > +				err = ice_set_promisc(vsi,
> > > +						      ICE_MCAST_PROMISC_BITS);
> > > +				if (err)
> > > +					goto out_promisc;
> > Aren't we already doing the same thing in case of IFF_ALLMULTI?
> > I wonder if our IFF_PROMISC handling is correct. Currently IFF_PROMISC
> > means setting PF VSI as default -> all packets from switch that don't
>                                                                   ^^^^^
> that's the critical failure of the original code.
> 
> > match any active rule goes to this VSI. If there is a rule (like in case
> > from the commit message) packet doesn't go to this VSI. Maybe it should
> > replicate all packets to PF VSI even there is a matching rule?
> 
> That's what this code change does, turn on the MCAST_PROMISC_BITS flag which
> makes the hardware replicate all multicasts received with *other*
> destination VSIs to this PF one.
> 
> > 
> > Basically, how IFF_PROMISC should work?
> 
> Yep, it's always how it should have worked, but it wasn't quite working
> right, and using bridge "fixed" it because bridge sets both PROMISC and
> ALLMULTI bits.
> 
> 

Ok, thanks for explanation.

From commit message:
"The root cause of the issue is that if the PF is asked to be in IFF_PROMISC
mode (promiscuous mode) and it had an ipv6 address that needed the
33:33:ff:00:00:04 multicast address to work, then when the VF was added
with the need for the same multicast address, the VF would steal all the
traffic destined for that address."

I wonder why there is no "perfect filter" for multicast ipv6 on PF if PF
have an ipv6 address? It is deleted by kernel in this configuration? Two
"perfect filters" for the same MAC results in packet replication (if
they have the same priority, but in our driver they have).
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Jesse Brandeburg]

On 2/9/2023 6:28 PM, Samudrala, Sridhar wrote:

>>> Basically, how IFF_PROMISC should work?
>>
>> Yep, it's always how it should have worked, but it wasn't quite 
>> working right, and using bridge "fixed" it because bridge sets both 
>> PROMISC and ALLMULTI bits.
> 
> Can this be solved by letting ovs set multicast snooping rather than 
> driver overloading IFF_PROMISC
>     ovs-vsctl set Bridge br0 mcast_snooping_enable=true

Might work? No one else has brought this up before now. However, I still 
think our driver is doing the wrong thing and this patch is needed.



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Jesse Brandeburg]

On 2/9/2023 10:05 PM, Michal Swiatkowski wrote:
> On Thu, Feb 09, 2023 at 06:08:45PM -0800, Jesse Brandeburg wrote:
>> Yep, it's always how it should have worked, but it wasn't quite working
>> right, and using bridge "fixed" it because bridge sets both PROMISC and
>> ALLMULTI bits.
>>
>>
> 
> Ok, thanks for explanation.
> 
>  From commit message:
> "The root cause of the issue is that if the PF is asked to be in IFF_PROMISC
> mode (promiscuous mode) and it had an ipv6 address that needed the
> 33:33:ff:00:00:04 multicast address to work, then when the VF was added
> with the need for the same multicast address, the VF would steal all the
> traffic destined for that address."
> 
> I wonder why there is no "perfect filter" for multicast ipv6 on PF if PF
> have an ipv6 address? It is deleted by kernel in this configuration? Two
> "perfect filters" for the same MAC results in packet replication (if
> they have the same priority, but in our driver they have).

If you have a look at the script I posted earlier in this thread, you'll 
see the tree of devices that was created.

PF ens259f0
   VLAN ens259f0.3 (VID=3)
     OVS ovs-br0 (child device ens259f0.3)
       IPv6 addr something::4

This combination ends up not creating perfect filters on the PF for the 
MAC address in question, maybe due to how OVS works?


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Samudrala, Sridhar]

On 2/10/2023 11:05 AM, Jesse Brandeburg wrote:
> On 2/9/2023 6:28 PM, Samudrala, Sridhar wrote:
> 
>>>> Basically, how IFF_PROMISC should work?
>>>
>>> Yep, it's always how it should have worked, but it wasn't quite 
>>> working right, and using bridge "fixed" it because bridge sets both 
>>> PROMISC and ALLMULTI bits.
>>
>> Can this be solved by letting ovs set multicast snooping rather than 
>> driver overloading IFF_PROMISC
>>     ovs-vsctl set Bridge br0 mcast_snooping_enable=true
> 
> Might work? No one else has brought this up before now. However, I still 
> think our driver is doing the wrong thing and this patch is needed.
>

Yes. As IFF_PROMISC implies multicast packets too, driver should
enable receiving multicast and your patch looks good.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Romanowski, Rafal]

> >> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and
> >> bump version")
> >> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >> ---
> >> v2: added fixes
> >> ---
> >>   drivers/net/ethernet/intel/ice/ice_main.c | 26
> >> +++++++++++++++++++++++
> >>   1 file changed, 26 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> >> b/drivers/net/ethernet/intel/ice/ice_main.c
> >> index 5f86e4111fa9..3a5f9c15b69c 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_main.c


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v2] ice: fix lost multicast packets in promisc mode

[Michal Swiatkowski]

On Fri, Feb 10, 2023 at 09:09:19AM -0800, Jesse Brandeburg wrote:
> On 2/9/2023 10:05 PM, Michal Swiatkowski wrote:
> > On Thu, Feb 09, 2023 at 06:08:45PM -0800, Jesse Brandeburg wrote:
> > > Yep, it's always how it should have worked, but it wasn't quite working
> > > right, and using bridge "fixed" it because bridge sets both PROMISC and
> > > ALLMULTI bits.
> > > 
> > > 
> > 
> > Ok, thanks for explanation.
> > 
> >  From commit message:
> > "The root cause of the issue is that if the PF is asked to be in IFF_PROMISC
> > mode (promiscuous mode) and it had an ipv6 address that needed the
> > 33:33:ff:00:00:04 multicast address to work, then when the VF was added
> > with the need for the same multicast address, the VF would steal all the
> > traffic destined for that address."
> > 
> > I wonder why there is no "perfect filter" for multicast ipv6 on PF if PF
> > have an ipv6 address? It is deleted by kernel in this configuration? Two
> > "perfect filters" for the same MAC results in packet replication (if
> > they have the same priority, but in our driver they have).
> 
> If you have a look at the script I posted earlier in this thread, you'll see
> the tree of devices that was created.
> 
> PF ens259f0
>   VLAN ens259f0.3 (VID=3)
>     OVS ovs-br0 (child device ens259f0.3)
>       IPv6 addr something::4
> 
> This combination ends up not creating perfect filters on the PF for the MAC
> address in question, maybe due to how OVS works?
> 
> 

Ok, understand, thanks.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan