Re: [PATCH v2 net-next 03/14] ipv6: prepare inet6_fill_ifinfo() for RCU protection

[Jiri Pirko <jiri@resnulli.us>]

Thu, Feb 22, 2024 at 11:50:10AM CET, edumazet@google.com wrote:
>We want to use RCU protection instead of RTNL

Is this a royal "We"? :)


>for inet6_fill_ifinfo().

This is a motivation for this patch, not what the patch does.

Would it be possible to maintain some sort of culture for the patch
descriptions, even of the patches which are small and simple?

https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes

Your patch descriptions are usually hard to follow for me to understand
what the patch does :( Yes, I know you do it "to displease me" as you
wrote couple of months ago but maybe think about the others too, also
the ones looking in a git log/show and guessing.

Don't beat me.


>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>---
> include/linux/netdevice.h |  6 ++++--
> net/core/dev.c            |  4 ++--
> net/ipv6/addrconf.c       | 11 +++++++----
> 3 files changed, 13 insertions(+), 8 deletions(-)
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index f07c8374f29cb936fe11236fc63e06e741b1c965..09023e44db4e2c3a2133afc52ba5a335d6030646 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -4354,8 +4354,10 @@ static inline bool netif_testing(const struct net_device *dev)
>  */
> static inline bool netif_oper_up(const struct net_device *dev)
> {
>-	return (dev->operstate == IF_OPER_UP ||
>-		dev->operstate == IF_OPER_UNKNOWN /* backward compat */);
>+	unsigned int operstate = READ_ONCE(dev->operstate);
>+
>+	return	operstate == IF_OPER_UP ||

double space  ^^


>+		operstate == IF_OPER_UNKNOWN /* backward compat */;
> }
> 
> /**
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 0628d8ff1ed932efdd45ab7b79599dcfcca6c4eb..275fd5259a4a92d0bd2e145d66a716248b6c2804 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -8632,12 +8632,12 @@ unsigned int dev_get_flags(const struct net_device *dev)
> {
> 	unsigned int flags;
> 
>-	flags = (dev->flags & ~(IFF_PROMISC |
>+	flags = (READ_ONCE(dev->flags) & ~(IFF_PROMISC |
> 				IFF_ALLMULTI |
> 				IFF_RUNNING |
> 				IFF_LOWER_UP |
> 				IFF_DORMANT)) |
>-		(dev->gflags & (IFF_PROMISC |
>+		(READ_ONCE(dev->gflags) & (IFF_PROMISC |
> 				IFF_ALLMULTI));
> 
> 	if (netif_running(dev)) {
>diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>index 3c8bdad0105dc9542489b612890ba86de9c44bdc..df3c6feea74e2d95144140eceb6df5cef2dce1f4 100644
>--- a/net/ipv6/addrconf.c
>+++ b/net/ipv6/addrconf.c
>@@ -6047,6 +6047,7 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
> 	struct net_device *dev = idev->dev;
> 	struct ifinfomsg *hdr;
> 	struct nlmsghdr *nlh;
>+	int ifindex, iflink;
> 	void *protoinfo;
> 
> 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*hdr), flags);
>@@ -6057,16 +6058,18 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
> 	hdr->ifi_family = AF_INET6;
> 	hdr->__ifi_pad = 0;
> 	hdr->ifi_type = dev->type;
>-	hdr->ifi_index = dev->ifindex;
>+	ifindex = READ_ONCE(dev->ifindex);
>+	hdr->ifi_index = ifindex;
> 	hdr->ifi_flags = dev_get_flags(dev);
> 	hdr->ifi_change = 0;
> 
>+	iflink = dev_get_iflink(dev);
> 	if (nla_put_string(skb, IFLA_IFNAME, dev->name) ||
> 	    (dev->addr_len &&
> 	     nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr)) ||
>-	    nla_put_u32(skb, IFLA_MTU, dev->mtu) ||
>-	    (dev->ifindex != dev_get_iflink(dev) &&
>-	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))) ||
>+	    nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
>+	    (ifindex != iflink &&
>+	     nla_put_u32(skb, IFLA_LINK, iflink)) ||
> 	    nla_put_u8(skb, IFLA_OPERSTATE,
> 		       netif_running(dev) ? READ_ONCE(dev->operstate) : IF_OPER_DOWN))
> 		goto nla_put_failure;
>-- 
>2.44.0.rc1.240.g4c46232300-goog
>
>