[PATCH net] enic: Avoid removing IPv6 address when updating rings size.

[PATCH net] enic: Avoid removing IPv6 address when updating rings size.

[Mohammad Heib]

Currently, the enic driver calls the dev_close function to temporarily
shut down the device before updating the device rings. This call
triggers a NETDEV_DOWN event, which is sent to the network stack via the
network notifier.

When the IPv6 stack receives such an event, it removes the IPv6
addresses from the affected device, keeping only the permanent
addresses. This behavior is inconsistent with other network drivers and
can lead to traffic loss, requiring reconfiguration of IPv6 addresses
after every ring update.

To avoid this behavior, this patch temporarily sets the interface config
`keep_addr_on_down` to 1 before closing the device during the rings
update, and restores the original value of `keep_addr_on_down` after
updating the device rings, this will prevent the ipv6 stack from
removing the current ipv6 addresses during the rings update.

Fixes: ed519b7488a4 ("enic: Add support for 'ethtool -g/-G'")
Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 .../net/ethernet/cisco/enic/enic_ethtool.c    | 27 ++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index d607b4f0542c..0860233ebac6 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -4,6 +4,7 @@
 #include <linux/netdevice.h>
 #include <linux/ethtool.h>
 #include <linux/net_tstamp.h>
+#include <net/addrconf.h>
 
 #include "enic_res.h"
 #include "enic.h"
@@ -235,10 +236,12 @@ static int enic_set_ringparam(struct net_device *netdev,
 {
 	struct enic *enic = netdev_priv(netdev);
 	struct vnic_enet_config *c = &enic->config;
+	struct inet6_dev *idev = NULL;
 	int running = netif_running(netdev);
 	unsigned int rx_pending;
 	unsigned int tx_pending;
 	int err = 0;
+	__s32 old_keep_addr_on_down = 0;
 
 	if (ring->rx_mini_max_pending || ring->rx_mini_pending) {
 		netdev_info(netdev,
@@ -266,8 +269,25 @@ static int enic_set_ringparam(struct net_device *netdev,
 			    ENIC_MAX_WQ_DESCS);
 		return -EINVAL;
 	}
-	if (running)
+	if (running) {
+		/* Temporarily store the old value of keep_addr_on_down for this specific
+		 * device and set it to 1. This ensures that the IPv6 stack call triggered
+		 * by the dev_close function in the next line will not remove the IPv6
+		 * addresses. The keep_addr_on_down value will be restored to its original
+		 * value before calling dev_open, ensuring that this temporary change does
+		 * not affect any future device changes.
+		 *
+		 * The rtnl lock was already acquired in the caller of this function,
+		 * so it is safe to call the function below.
+		 */
+		idev = __in6_dev_get(netdev);
+		if (idev) {
+			old_keep_addr_on_down = idev->cnf.keep_addr_on_down;
+			idev->cnf.keep_addr_on_down = 1;
+		}
 		dev_close(netdev);
+	}
+
 	c->rq_desc_count =
 		ring->rx_pending & 0xffffffe0; /* must be aligned to groups of 32 */
 	c->wq_desc_count =
@@ -282,6 +302,9 @@ static int enic_set_ringparam(struct net_device *netdev,
 	}
 	enic_init_vnic_resources(enic);
 	if (running) {
+		if (idev)
+			idev->cnf.keep_addr_on_down = old_keep_addr_on_down;
+
 		err = dev_open(netdev, NULL);
 		if (err)
 			goto err_out;
@@ -290,6 +313,8 @@ static int enic_set_ringparam(struct net_device *netdev,
 err_out:
 	c->rq_desc_count = rx_pending;
 	c->wq_desc_count = tx_pending;
+	if (idev)
+		idev->cnf.keep_addr_on_down = old_keep_addr_on_down;
 	return err;
 }
 
-- 
2.48.1

Re: [PATCH net] enic: Avoid removing IPv6 address when updating rings size.

[Michal Schmidt]

On Sun, Feb 23, 2025 at 10:42 PM Mohammad Heib <mheib@redhat.com> wrote:
> Currently, the enic driver calls the dev_close function to temporarily
> shut down the device before updating the device rings. This call
> triggers a NETDEV_DOWN event, which is sent to the network stack via the
> network notifier.
>
> When the IPv6 stack receives such an event, it removes the IPv6
> addresses from the affected device, keeping only the permanent
> addresses. This behavior is inconsistent with other network drivers and
> can lead to traffic loss, requiring reconfiguration of IPv6 addresses
> after every ring update.
>
> To avoid this behavior, this patch temporarily sets the interface config
> `keep_addr_on_down` to 1 before closing the device during the rings
> update, and restores the original value of `keep_addr_on_down` after
> updating the device rings, this will prevent the ipv6 stack from
> removing the current ipv6 addresses during the rings update.

That's icky. Can you instead avoid using the dev_close()/dev_open()?
Michal

Re: [PATCH net] enic: Avoid removing IPv6 address when updating rings size.

[Jakub Kicinski]

On Sun, 23 Feb 2025 23:42:03 +0200 Mohammad Heib wrote:
> To avoid this behavior, this patch temporarily sets the interface config
> `keep_addr_on_down` to 1 before closing the device during the rings
> update, and restores the original value of `keep_addr_on_down` after
> updating the device rings, this will prevent the ipv6 stack from
> removing the current ipv6 addresses during the rings update.

Wrong direction, you should support reconfiguration natively 
in the driver. Pre-allocate all the resources for the new config
and swap it with the running one.
-- 
pw-bot: cr