[Patch] net: kill an RCU warning in inet_fill_link_af()

[Patch] net: kill an RCU warning in inet_fill_link_af()

[Amerigo Wang]

From: WANG Cong <amwang@redhat.com>

The latest net-next-2.6 triggers an RCU warning during boot,
lockdep complains that in inet_fill_link_af() we call rcu_dereference_check()
without rcu_read_lock() protection.

This patch fixes it by replacing __in_dev_get_rcu() with in_dev_get().

Signed-off-by: WANG Cong <amwang@redhat.com>

---
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d9f71ba..73baed8 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1258,31 +1258,36 @@ errout:
 
 static size_t inet_get_link_af_size(const struct net_device *dev)
 {
-	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct in_device *in_dev = in_dev_get(dev);
 
 	if (!in_dev)
 		return 0;
 
+	in_dev_put(in_dev);
 	return nla_total_size(IPV4_DEVCONF_MAX * 4); /* IFLA_INET_CONF */
 }
 
 static int inet_fill_link_af(struct sk_buff *skb, const struct net_device *dev)
 {
-	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct in_device *in_dev = in_dev_get(dev);
 	struct nlattr *nla;
-	int i;
+	int i, ret = 0;
 
 	if (!in_dev)
 		return -ENODATA;
 
 	nla = nla_reserve(skb, IFLA_INET_CONF, IPV4_DEVCONF_MAX * 4);
-	if (nla == NULL)
-		return -EMSGSIZE;
+	if (nla == NULL) {
+		ret = -EMSGSIZE;
+		goto out;
+	}
 
 	for (i = 0; i < IPV4_DEVCONF_MAX; i++)
 		((u32 *) nla_data(nla))[i] = in_dev->cnf.data[i];
 
-	return 0;
+out:
+	in_dev_put(in_dev);
+	return ret;
 }
 
 static const struct nla_policy inet_af_policy[IFLA_INET_MAX+1] = {
@@ -1293,11 +1298,14 @@ static int inet_validate_link_af(const struct net_device *dev,
 				 const struct nlattr *nla)
 {
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
+	struct in_device *in_dev = in_dev_get(dev);
 	int err, rem;
 
-	if (dev && !__in_dev_get_rcu(dev))
+	if (dev && !in_dev)
 		return -EAFNOSUPPORT;
 
+	in_dev_put(in_dev);
+
 	err = nla_parse_nested(tb, IFLA_INET_MAX, nla, inet_af_policy);
 	if (err < 0)
 		return err;
@@ -1319,7 +1327,7 @@ static int inet_validate_link_af(const struct net_device *dev,
 
 static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
 {
-	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct in_device *in_dev = in_dev_get(dev);
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
 	int rem;
 
@@ -1334,6 +1342,7 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
 			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
 	}
 
+	in_dev_put(in_dev);
 	return 0;
 }
 

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[Eric Dumazet]

Le mercredi 01 décembre 2010 à 19:14 +0800, Amerigo Wang a écrit :
> From: WANG Cong <amwang@redhat.com>
> 
> The latest net-next-2.6 triggers an RCU warning during boot,
> lockdep complains that in inet_fill_link_af() we call rcu_dereference_check()
> without rcu_read_lock() protection.
> 
> This patch fixes it by replacing __in_dev_get_rcu() with in_dev_get().
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> 

Sorry patch is not the right fix. Please take a look at commit 95ae6b22

We are working hard to remove all the not needed get()/put(), not to add
new ones ;)


> ---
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index d9f71ba..73baed8 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1258,31 +1258,36 @@ errout:
>  
>  static size_t inet_get_link_af_size(const struct net_device *dev)
>  {
> -	struct in_device *in_dev = __in_dev_get_rcu(dev);
> +	struct in_device *in_dev = in_dev_get(dev);
>  
>  	if (!in_dev)
>  		return 0;
>  
> +	in_dev_put(in_dev);
>  	return nla_total_size(IPV4_DEVCONF_MAX * 4); /* IFLA_INET_CONF */
>  }
>  

In this function why should we even take a reference, just to check if
pointer exists ?

If RTNL is held (I believe so), just use __in_dev_get_rtnl()

>  static int inet_fill_link_af(struct sk_buff *skb, const struct net_device *dev)
>  {
> -	struct in_device *in_dev = __in_dev_get_rcu(dev);
> +	struct in_device *in_dev = in_dev_get(dev);
>  	struct nlattr *nla;
> -	int i;
> +	int i, ret = 0;
>  
>  	if (!in_dev)
>  		return -ENODATA;
>  
>  	nla = nla_reserve(skb, IFLA_INET_CONF, IPV4_DEVCONF_MAX * 4);
> -	if (nla == NULL)
> -		return -EMSGSIZE;
> +	if (nla == NULL) {
> +		ret = -EMSGSIZE;
> +		goto out;
> +	}
>  
>  	for (i = 0; i < IPV4_DEVCONF_MAX; i++)
>  		((u32 *) nla_data(nla))[i] = in_dev->cnf.data[i];
>  
> -	return 0;
> +out:
> +	in_dev_put(in_dev);
> +	return ret;
>  }


In this function we hold RTNL...
 Please use __in_dev_get_rtnl()


>  
>  static const struct nla_policy inet_af_policy[IFLA_INET_MAX+1] = {
> @@ -1293,11 +1298,14 @@ static int inet_validate_link_af(const struct net_device *dev,
>  				 const struct nlattr *nla)
>  {
>  	struct nlattr *a, *tb[IFLA_INET_MAX+1];
> +	struct in_device *in_dev = in_dev_get(dev);
>  	int err, rem;
>  
> -	if (dev && !__in_dev_get_rcu(dev))
> +	if (dev && !in_dev)
>  		return -EAFNOSUPPORT;
>  
> +	in_dev_put(in_dev);
> +
>  	err = nla_parse_nested(tb, IFLA_INET_MAX, nla, inet_af_policy);
>  	if (err < 0)
>  		return err;
> @@ -1319,7 +1327,7 @@ static int inet_validate_link_af(const struct net_device *dev,
>  
>  static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
>  {
> -	struct in_device *in_dev = __in_dev_get_rcu(dev);
> +	struct in_device *in_dev = in_dev_get(dev);
>  	struct nlattr *a, *tb[IFLA_INET_MAX+1];
>  	int rem;
>  
> @@ -1334,6 +1342,7 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
>  			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
>  	}
>  
> +	in_dev_put(in_dev);
>  	return 0;
>  }

Same here. RTNL is held. Please use __in_dev_get_rtnl()

[PATCH net-next-2.6] : __in_dev_get_rtnl() can use rtnl_dereference()

[Eric Dumazet]

Le mercredi 01 décembre 2010 à 12:21 +0100, Eric Dumazet a écrit :

> Same here. RTNL is held. Please use __in_dev_get_rtnl()

By the way we can use rtnl_dereference() in __in_dev_get_rtnl()

[PATCH net-next-2.6] : __in_dev_get_rtnl() can use rtnl_dereference()

If caller holds RTNL, we dont need a memory barrier
(smp_read_barrier_depends) included in rcu_dereference().

Just use rtnl_dereference() to properly document the assertions.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/inetdevice.h |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 2b86eaf..ae8fdc5 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -222,7 +222,7 @@ static inline struct in_device *in_dev_get(const struct net_device *dev)
 
 static inline struct in_device *__in_dev_get_rtnl(const struct net_device *dev)
 {
-	return rcu_dereference_check(dev->ip_ptr, lockdep_rtnl_is_held());
+	return rtnl_dereference(dev->ip_ptr);
 }
 
 extern void in_dev_finish_destroy(struct in_device *idev);

Re: [PATCH net-next-2.6] : __in_dev_get_rtnl() can use rtnl_dereference()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 01 Dec 2010 12:37:42 +0100

> Le mercredi 01 décembre 2010 à 12:21 +0100, Eric Dumazet a écrit :
> 
>> Same here. RTNL is held. Please use __in_dev_get_rtnl()
> 
> By the way we can use rtnl_dereference() in __in_dev_get_rtnl()
> 
> [PATCH net-next-2.6] : __in_dev_get_rtnl() can use rtnl_dereference()
> 
> If caller holds RTNL, we dont need a memory barrier
> (smp_read_barrier_depends) included in rcu_dereference().
> 
> Just use rtnl_dereference() to properly document the assertions.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[Eric Dumazet]

Le mercredi 01 décembre 2010 à 19:14 +0800, Amerigo Wang a écrit :
> From: WANG Cong <amwang@redhat.com>
> 
> The latest net-next-2.6 triggers an RCU warning during boot,
> lockdep complains that in inet_fill_link_af() we call rcu_dereference_check()
> without rcu_read_lock() protection.
> 
> This patch fixes it by replacing __in_dev_get_rcu() with in_dev_get().

Here is a better version, thanks a lot for your report and initial
patch.


[PATCH net-next-2.6] net: kill an RCU warning in inet_fill_link_af()

commits 9f0f7272 (ipv4: AF_INET link address family) and cf7afbfeb8c
(rtnl: make link af-specific updates atomic) used incorrect
__in_dev_get_rcu() in RTNL protected contexts, triggering PROVE_RCU
warnings.

Switch to __in_dev_get_rtnl(), wich is more appropriate, since we hold
RTNL.

Based on a report and initial patch from Amerigo Wang.

Reported-by: Amerigo Wang <amwang@redhat.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Thomas Graf <tgraf@infradead.org>
---
 net/ipv4/devinet.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d9f71ba..3b06770 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1258,7 +1258,7 @@ errout:
 
 static size_t inet_get_link_af_size(const struct net_device *dev)
 {
-	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct in_device *in_dev = __in_dev_get_rtnl(dev);
 
 	if (!in_dev)
 		return 0;
@@ -1268,7 +1268,7 @@ static size_t inet_get_link_af_size(const struct net_device *dev)
 
 static int inet_fill_link_af(struct sk_buff *skb, const struct net_device *dev)
 {
-	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct in_device *in_dev = __in_dev_get_rtnl(dev);
 	struct nlattr *nla;
 	int i;
 
@@ -1295,7 +1295,7 @@ static int inet_validate_link_af(const struct net_device *dev,
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
 	int err, rem;
 
-	if (dev && !__in_dev_get_rcu(dev))
+	if (dev && !__in_dev_get_rtnl(dev))
 		return -EAFNOSUPPORT;
 
 	err = nla_parse_nested(tb, IFLA_INET_MAX, nla, inet_af_policy);
@@ -1319,7 +1319,7 @@ static int inet_validate_link_af(const struct net_device *dev,
 
 static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
 {
-	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct in_device *in_dev = __in_dev_get_rtnl(dev);
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
 	int rem;
 

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[Thomas Graf]

On Wed, Dec 01, 2010 at 05:03:06PM +0100, Eric Dumazet wrote:
> [PATCH net-next-2.6] net: kill an RCU warning in inet_fill_link_af()
> 
> commits 9f0f7272 (ipv4: AF_INET link address family) and cf7afbfeb8c
> (rtnl: make link af-specific updates atomic) used incorrect
> __in_dev_get_rcu() in RTNL protected contexts, triggering PROVE_RCU
> warnings.
> 
> Switch to __in_dev_get_rtnl(), wich is more appropriate, since we hold
> RTNL.
> 
> Based on a report and initial patch from Amerigo Wang.

RTNL is not held while dumping, it is only held for get and set, but we
still hold rcu readlocks while dumping so there should be no asserts
triggered. Thanks for fixing this.

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[Eric Dumazet]

Le mercredi 01 décembre 2010 à 12:18 -0500, Thomas Graf a écrit :
> On Wed, Dec 01, 2010 at 05:03:06PM +0100, Eric Dumazet wrote:
> > [PATCH net-next-2.6] net: kill an RCU warning in inet_fill_link_af()
> > 
> > commits 9f0f7272 (ipv4: AF_INET link address family) and cf7afbfeb8c
> > (rtnl: make link af-specific updates atomic) used incorrect
> > __in_dev_get_rcu() in RTNL protected contexts, triggering PROVE_RCU
> > warnings.
> > 
> > Switch to __in_dev_get_rtnl(), wich is more appropriate, since we hold
> > RTNL.
> > 
> > Based on a report and initial patch from Amerigo Wang.
> 
> RTNL is not held while dumping, it is only held for get and set, but we
> still hold rcu readlocks while dumping so there should be no asserts
> triggered. Thanks for fixing this.

Are you sure RTNL is not held while dumping ? 

Patrick did the change to hold RTNL while dumping too, 3.5 years ago.
Check commits 6313c1e0992fea, 1c2d670f3660e9103 ([RTNETLINK]: Hold
rtnl_mutex during netlink dump callbacks)

If this was the case (not holding RTNL), we should use
rcu_dereference(), not rtnl_dereference()

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[Thomas Graf]

On Wed, Dec 01, 2010 at 06:31:57PM +0100, Eric Dumazet wrote:
> Are you sure RTNL is not held while dumping ? 
> 
> Patrick did the change to hold RTNL while dumping too, 3.5 years ago.
> Check commits 6313c1e0992fea, 1c2d670f3660e9103 ([RTNETLINK]: Hold
> rtnl_mutex during netlink dump callbacks)

You are right, I only looked at netlink_dump_start() where the lock
is released after setting ->cb.  We grab RTNL again in netlink_dump()
after allocating the skb.

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[Cong Wang]

On 12/02/10 00:03, Eric Dumazet wrote:
> Le mercredi 01 décembre 2010 à 19:14 +0800, Amerigo Wang a écrit :
>> From: WANG Cong<amwang@redhat.com>
>>
>> The latest net-next-2.6 triggers an RCU warning during boot,
>> lockdep complains that in inet_fill_link_af() we call rcu_dereference_check()
>> without rcu_read_lock() protection.
>>
>> This patch fixes it by replacing __in_dev_get_rcu() with in_dev_get().
>
> Here is a better version, thanks a lot for your report and initial
> patch.
>
>
> [PATCH net-next-2.6] net: kill an RCU warning in inet_fill_link_af()
>
> commits 9f0f7272 (ipv4: AF_INET link address family) and cf7afbfeb8c
> (rtnl: make link af-specific updates atomic) used incorrect
> __in_dev_get_rcu() in RTNL protected contexts, triggering PROVE_RCU
> warnings.
>
> Switch to __in_dev_get_rtnl(), wich is more appropriate, since we hold
> RTNL.
>
> Based on a report and initial patch from Amerigo Wang.
>

Alright, thanks for fixing it.

Reviewed-by: WANG Cong <amwang@redhat.com>

Re: [Patch] net: kill an RCU warning in inet_fill_link_af()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 01 Dec 2010 17:03:06 +0100

> Le mercredi 01 décembre 2010 à 19:14 +0800, Amerigo Wang a écrit :
>> From: WANG Cong <amwang@redhat.com>
>> 
>> The latest net-next-2.6 triggers an RCU warning during boot,
>> lockdep complains that in inet_fill_link_af() we call rcu_dereference_check()
>> without rcu_read_lock() protection.
>> 
>> This patch fixes it by replacing __in_dev_get_rcu() with in_dev_get().
> 
> Here is a better version, thanks a lot for your report and initial
> patch.
> 
> 
> [PATCH net-next-2.6] net: kill an RCU warning in inet_fill_link_af()
> 
> commits 9f0f7272 (ipv4: AF_INET link address family) and cf7afbfeb8c
> (rtnl: make link af-specific updates atomic) used incorrect
> __in_dev_get_rcu() in RTNL protected contexts, triggering PROVE_RCU
> warnings.
> 
> Switch to __in_dev_get_rtnl(), wich is more appropriate, since we hold
> RTNL.
> 
> Based on a report and initial patch from Amerigo Wang.
> 
> Reported-by: Amerigo Wang <amwang@redhat.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Thomas Graf <tgraf@infradead.org>

Applied.