Skip to contentSkip to conversation

ip: Random cleanup for devinet.c

PATCH Applied Kuniyuki Iwashima 2 participants 7 messages Aug 9, 2024 – Aug 16, 2024

Subsystems (5)
Networking Network drivers IPv4/IPv6 QLogic qlcnic (1/10)Gb Ethernet driver +1 more subsystems
lore
Share & tools
lore

Kuniyuki Iwashima

author
#0 lore raw

cover ip: Random cleanup for devinet.c

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(-)
Signature · 1 line
2.30.2
author
#1 lore raw

PATCH 1/5 ipv4: Check !in_dev earlier for ioctl(SIOCSIFADDR).

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 <[email protected]>
---
 net/ipv4/devinet.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
net/ipv4/devinet.c +2 -4
index d96f3e452fef..ddab15116454 100644
@@ -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;
Signature · 1 line
2.30.2
author
#2 lore raw

PATCH 2/5 ipv4: Set ifa->ifa_dev in inet_alloc_ifa().

In reply to 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 <[email protected]>
---
 net/ipv4/devinet.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
net/ipv4/devinet.c +15 -15
index ddab15116454..9f4add07e67d 100644
@@ -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,
Signature · 1 line
2.30.2
author
#3 lore raw

PATCH 3/5 ipv4: Remove redundant !ifa->ifa_dev check.

In reply to 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 <[email protected]>

---
CC: Manish Chopra <[email protected]>
CC: Rahul Verma <[email protected]>
CC: [email protected]

---
 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(-)
drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c +2 -3
index ed24d6af7487..9cff0a8ffb2c 100644
@@ -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)
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c +1 -1
index 90df4a0909fa..b3588a1ebc25 100644
@@ -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)
net/ipv4/devinet.c +1 -2
index 9f4add07e67d..baf036bfad76 100644
@@ -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);
 }
Signature · 1 line
2.30.2
author
#4 lore raw

PATCH 4/5 ipv4: Initialise ifa->hash in inet_alloc_ifa().

In reply to 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 <[email protected]>
---
 net/ipv4/devinet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
net/ipv4/devinet.c +3 -3
index baf036bfad76..b5d2a9fd46c7 100644
@@ -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;
Signature · 1 line
2.30.2
author
#5 lore raw

PATCH 5/5 ip: Move INFINITY_LIFE_TIME to addrconf.h.

In reply to 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 <[email protected]>
---
 include/net/addrconf.h | 4 +++-
 net/ipv4/devinet.c     | 2 --
 net/ipv6/addrconf.c    | 2 --
 3 files changed, 3 insertions(+), 5 deletions(-)
include/net/addrconf.h +3 -1
index b18e81f0c9e1..c8ed31828db3 100644
@@ -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;
 
 	/*
net/ipv4/devinet.c +0 -2
index b5d2a9fd46c7..61be85154dd1 100644
@@ -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;
net/ipv6/addrconf.c +0 -2
index c87d008aefa4..04ee75af2f6b 100644
@@ -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")
Signature · 1 line
2.30.2
CI Applied
#6 lore raw
In reply to Kuniyuki Iwashima
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 4 lines · 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.

Quoted 8 lines

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!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html