[PATCH v1 net-next 0/5] ip: Random cleanup for devinet.c

[PATCH v1 net-next 0/5] ip: Random cleanup for devinet.c

[Kuniyuki Iwashima]

patch 1 ~ 3 remove defensive !ifa->ifa_dev tests.
patch 4 & 5 deduplicate common code.


Kuniyuki Iwashima (5):
  ipv4: Check !in_dev earlier for ioctl(SIOCSIFADDR).
  ipv4: Set ifa->ifa_dev in inet_alloc_ifa().
  ipv4: Remove redundant !ifa->ifa_dev check.
  ipv4: Initialise ifa->hash in inet_alloc_ifa().
  ip: Move INFINITY_LIFE_TIME to addrconf.h.

 .../ethernet/qlogic/netxen/netxen_nic_main.c  |  5 +-
 .../net/ethernet/qlogic/qlcnic/qlcnic_main.c  |  2 +-
 include/net/addrconf.h                        |  4 +-
 net/ipv4/devinet.c                            | 47 +++++++++----------
 net/ipv6/addrconf.c                           |  2 -
 5 files changed, 27 insertions(+), 33 deletions(-)

-- 
2.30.2

[PATCH v1 net-next 1/5] ipv4: Check !in_dev earlier for ioctl(SIOCSIFADDR).

[Kuniyuki Iwashima]

dev->ip_ptr could be NULL if we set an invalid MTU.

Even then, if we issue ioctl(SIOCSIFADDR) for a new IPv4 address,
devinet_ioctl() allocates struct in_ifaddr and fails later in
inet_set_ifa() because in_dev is NULL.

Let's move the check earlier.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/devinet.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d96f3e452fef..ddab15116454 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -574,10 +574,6 @@ static int inet_set_ifa(struct net_device *dev, struct in_ifaddr *ifa)
 
 	ASSERT_RTNL();
 
-	if (!in_dev) {
-		inet_free_ifa(ifa);
-		return -ENOBUFS;
-	}
 	ipv4_devconf_setall(in_dev);
 	neigh_parms_data_state_setall(in_dev->arp_parms);
 	if (ifa->ifa_dev != in_dev) {
@@ -1184,6 +1180,8 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 
 		if (!ifa) {
 			ret = -ENOBUFS;
+			if (!in_dev)
+				break;
 			ifa = inet_alloc_ifa();
 			if (!ifa)
 				break;
-- 
2.30.2

[PATCH v1 net-next 2/5] ipv4: Set ifa->ifa_dev in inet_alloc_ifa().

[Kuniyuki Iwashima]

When a new IPv4 address is assigned via ioctl(SIOCSIFADDR),
inet_set_ifa() sets ifa->ifa_dev if it's different from in_dev
passed as an argument.

In this case, ifa is always a newly allocated object, and
ifa->ifa_dev is NULL.

inet_set_ifa() can be called for an existing reused ifa, then,
this check is always false.

Let's set ifa_dev in inet_alloc_ifa() and remove the check
in inet_set_ifa().

Now, inet_alloc_ifa() is symmetric with inet_rcu_free_ifa().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/devinet.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index ddab15116454..9f4add07e67d 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -216,9 +216,18 @@ static void devinet_sysctl_unregister(struct in_device *idev)
 
 /* Locks all the inet devices. */
 
-static struct in_ifaddr *inet_alloc_ifa(void)
+static struct in_ifaddr *inet_alloc_ifa(struct in_device *in_dev)
 {
-	return kzalloc(sizeof(struct in_ifaddr), GFP_KERNEL_ACCOUNT);
+	struct in_ifaddr *ifa;
+
+	ifa = kzalloc(sizeof(*ifa), GFP_KERNEL_ACCOUNT);
+	if (!ifa)
+		return NULL;
+
+	in_dev_hold(in_dev);
+	ifa->ifa_dev = in_dev;
+
+	return ifa;
 }
 
 static void inet_rcu_free_ifa(struct rcu_head *head)
@@ -576,11 +585,7 @@ static int inet_set_ifa(struct net_device *dev, struct in_ifaddr *ifa)
 
 	ipv4_devconf_setall(in_dev);
 	neigh_parms_data_state_setall(in_dev->arp_parms);
-	if (ifa->ifa_dev != in_dev) {
-		WARN_ON(ifa->ifa_dev);
-		in_dev_hold(in_dev);
-		ifa->ifa_dev = in_dev;
-	}
+
 	if (ipv4_is_loopback(ifa->ifa_local))
 		ifa->ifa_scope = RT_SCOPE_HOST;
 	return inet_insert_ifa(ifa);
@@ -871,7 +876,7 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,
 	if (!in_dev)
 		goto errout;
 
-	ifa = inet_alloc_ifa();
+	ifa = inet_alloc_ifa(in_dev);
 	if (!ifa)
 		/*
 		 * A potential indev allocation can be left alive, it stays
@@ -881,7 +886,6 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,
 
 	ipv4_devconf_setall(in_dev);
 	neigh_parms_data_state_setall(in_dev->arp_parms);
-	in_dev_hold(in_dev);
 
 	if (!tb[IFA_ADDRESS])
 		tb[IFA_ADDRESS] = tb[IFA_LOCAL];
@@ -892,8 +896,6 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,
 	ifa->ifa_flags = tb[IFA_FLAGS] ? nla_get_u32(tb[IFA_FLAGS]) :
 					 ifm->ifa_flags;
 	ifa->ifa_scope = ifm->ifa_scope;
-	ifa->ifa_dev = in_dev;
-
 	ifa->ifa_local = nla_get_in_addr(tb[IFA_LOCAL]);
 	ifa->ifa_address = nla_get_in_addr(tb[IFA_ADDRESS]);
 
@@ -1182,7 +1184,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 			ret = -ENOBUFS;
 			if (!in_dev)
 				break;
-			ifa = inet_alloc_ifa();
+			ifa = inet_alloc_ifa(in_dev);
 			if (!ifa)
 				break;
 			INIT_HLIST_NODE(&ifa->hash);
@@ -1584,7 +1586,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 		if (!inetdev_valid_mtu(dev->mtu))
 			break;
 		if (dev->flags & IFF_LOOPBACK) {
-			struct in_ifaddr *ifa = inet_alloc_ifa();
+			struct in_ifaddr *ifa = inet_alloc_ifa(in_dev);
 
 			if (ifa) {
 				INIT_HLIST_NODE(&ifa->hash);
@@ -1592,8 +1594,6 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 				  ifa->ifa_address = htonl(INADDR_LOOPBACK);
 				ifa->ifa_prefixlen = 8;
 				ifa->ifa_mask = inet_make_mask(8);
-				in_dev_hold(in_dev);
-				ifa->ifa_dev = in_dev;
 				ifa->ifa_scope = RT_SCOPE_HOST;
 				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
 				set_ifa_lifetime(ifa, INFINITY_LIFE_TIME,
-- 
2.30.2

[PATCH v1 net-next 3/5] ipv4: Remove redundant !ifa->ifa_dev check.

[Kuniyuki Iwashima]

Now, ifa_dev is only set in inet_alloc_ifa() and never
NULL after ifa gets visible.

Let's remove the unneeded NULL check for ifa->ifa_dev.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
CC: Manish Chopra <manishc@marvell.com>
CC: Rahul Verma <rahulv@marvell.com>
CC: PGR-Linux-NIC-Dev@marvell.com
---
 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c | 5 ++---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c     | 2 +-
 net/ipv4/devinet.c                                   | 3 +--
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index ed24d6af7487..9cff0a8ffb2c 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -3185,8 +3185,7 @@ netxen_list_config_ip(struct netxen_adapter *adapter,
 	struct list_head *head;
 	bool ret = false;
 
-	dev = ifa->ifa_dev ? ifa->ifa_dev->dev : NULL;
-
+	dev = ifa->ifa_dev->dev;
 	if (dev == NULL)
 		goto out;
 
@@ -3379,7 +3378,7 @@ netxen_inetaddr_event(struct notifier_block *this,
 	struct in_ifaddr *ifa = (struct in_ifaddr *)ptr;
 	unsigned long ip_event;
 
-	dev = ifa->ifa_dev ? ifa->ifa_dev->dev : NULL;
+	dev = ifa->ifa_dev->dev;
 	ip_event = (event == NETDEV_UP) ? NX_IP_UP : NX_IP_DOWN;
 recheck:
 	if (dev == NULL)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 90df4a0909fa..b3588a1ebc25 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -4146,7 +4146,7 @@ qlcnic_inetaddr_event(struct notifier_block *this,
 
 	struct in_ifaddr *ifa = (struct in_ifaddr *)ptr;
 
-	dev = ifa->ifa_dev ? ifa->ifa_dev->dev : NULL;
+	dev = ifa->ifa_dev->dev;
 
 recheck:
 	if (dev == NULL)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 9f4add07e67d..baf036bfad76 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -234,8 +234,7 @@ static void inet_rcu_free_ifa(struct rcu_head *head)
 {
 	struct in_ifaddr *ifa = container_of(head, struct in_ifaddr, rcu_head);
 
-	if (ifa->ifa_dev)
-		in_dev_put(ifa->ifa_dev);
+	in_dev_put(ifa->ifa_dev);
 	kfree(ifa);
 }
 
-- 
2.30.2

[PATCH v1 net-next 4/5] ipv4: Initialise ifa->hash in inet_alloc_ifa().

[Kuniyuki Iwashima]

Whenever ifa is allocated, we call INIT_HLIST_NODE(&ifa->hash).

Let's move it to inet_alloc_ifa().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/devinet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index baf036bfad76..b5d2a9fd46c7 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -227,6 +227,8 @@ static struct in_ifaddr *inet_alloc_ifa(struct in_device *in_dev)
 	in_dev_hold(in_dev);
 	ifa->ifa_dev = in_dev;
 
+	INIT_HLIST_NODE(&ifa->hash);
+
 	return ifa;
 }
 
@@ -889,7 +891,6 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,
 	if (!tb[IFA_ADDRESS])
 		tb[IFA_ADDRESS] = tb[IFA_LOCAL];
 
-	INIT_HLIST_NODE(&ifa->hash);
 	ifa->ifa_prefixlen = ifm->ifa_prefixlen;
 	ifa->ifa_mask = inet_make_mask(ifm->ifa_prefixlen);
 	ifa->ifa_flags = tb[IFA_FLAGS] ? nla_get_u32(tb[IFA_FLAGS]) :
@@ -1186,7 +1187,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 			ifa = inet_alloc_ifa(in_dev);
 			if (!ifa)
 				break;
-			INIT_HLIST_NODE(&ifa->hash);
+
 			if (colon)
 				memcpy(ifa->ifa_label, ifr->ifr_name, IFNAMSIZ);
 			else
@@ -1588,7 +1589,6 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 			struct in_ifaddr *ifa = inet_alloc_ifa(in_dev);
 
 			if (ifa) {
-				INIT_HLIST_NODE(&ifa->hash);
 				ifa->ifa_local =
 				  ifa->ifa_address = htonl(INADDR_LOOPBACK);
 				ifa->ifa_prefixlen = 8;
-- 
2.30.2

[PATCH v1 net-next 5/5] ip: Move INFINITY_LIFE_TIME to addrconf.h.

[Kuniyuki Iwashima]

INFINITY_LIFE_TIME is the common value used in IPv4 and IPv6 but defined
in both .c files.

Also, 0xffffffff used in addrconf_timeout_fixup() is INFINITY_LIFE_TIME.

Let's move INFINITY_LIFE_TIME's definition to addrconf.h

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/addrconf.h | 4 +++-
 net/ipv4/devinet.c     | 2 --
 net/ipv6/addrconf.c    | 2 --
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index b18e81f0c9e1..c8ed31828db3 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -187,10 +187,12 @@ static inline int addrconf_ifid_eui48(u8 *eui, struct net_device *dev)
 	return 0;
 }
 
+#define INFINITY_LIFE_TIME 0xFFFFFFFF
+
 static inline unsigned long addrconf_timeout_fixup(u32 timeout,
 						   unsigned int unit)
 {
-	if (timeout == 0xffffffff)
+	if (timeout == INFINITY_LIFE_TIME)
 		return ~0UL;
 
 	/*
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index b5d2a9fd46c7..61be85154dd1 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -703,8 +703,6 @@ static int inet_rtm_deladdr(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-#define INFINITY_LIFE_TIME	0xFFFFFFFF
-
 static void check_lifetime(struct work_struct *work)
 {
 	unsigned long now, next, next_sec, next_sched;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index c87d008aefa4..04ee75af2f6b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -92,8 +92,6 @@
 #include <linux/export.h>
 #include <linux/ioam6.h>
 
-#define	INFINITY_LIFE_TIME	0xFFFFFFFF
-
 #define IPV6_MAX_STRLEN \
 	sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")
 
-- 
2.30.2

Re: [PATCH v1 net-next 0/5] ip: Random cleanup for devinet.c

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 9 Aug 2024 16:54:01 -0700 you wrote:
> patch 1 ~ 3 remove defensive !ifa->ifa_dev tests.
> patch 4 & 5 deduplicate common code.
> 
> 
> Kuniyuki Iwashima (5):
>   ipv4: Check !in_dev earlier for ioctl(SIOCSIFADDR).
>   ipv4: Set ifa->ifa_dev in inet_alloc_ifa().
>   ipv4: Remove redundant !ifa->ifa_dev check.
>   ipv4: Initialise ifa->hash in inet_alloc_ifa().
>   ip: Move INFINITY_LIFE_TIME to addrconf.h.
> 
> [...]

Here is the summary with links:
  - [v1,net-next,1/5] ipv4: Check !in_dev earlier for ioctl(SIOCSIFADDR).
    https://git.kernel.org/netdev/net-next/c/e3af3d3c5b26
  - [v1,net-next,2/5] ipv4: Set ifa->ifa_dev in inet_alloc_ifa().
    https://git.kernel.org/netdev/net-next/c/6e701eb91412
  - [v1,net-next,3/5] ipv4: Remove redundant !ifa->ifa_dev check.
    https://git.kernel.org/netdev/net-next/c/ecdae5168460
  - [v1,net-next,4/5] ipv4: Initialise ifa->hash in inet_alloc_ifa().
    https://git.kernel.org/netdev/net-next/c/100465a91a90
  - [v1,net-next,5/5] ip: Move INFINITY_LIFE_TIME to addrconf.h.
    https://git.kernel.org/netdev/net-next/c/de67763cbdbb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html