[RFC] ifa_list needs proper rcu protection

[RFC] ifa_list needs proper rcu protection

[Eric Dumazet]

It looks that unless RTNL is held, accessing ifa_list needs proper RCU protection ?

indev->ifa_list can be changed under us by another cpu (which owns RTNL)

Lets took an example.

(A proper rcu_dereference() with an happy sparse support would require adding __rcu attribute,
 I put a READ_ONCE() which should be just fine in this particular context)

diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index 78a9e6454ff3d712926397beb904b478b8fab0f1..8619b8d02b0530c5735c31d029f1d79969d979c7 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -48,14 +48,17 @@ nf_nat_redirect_ipv4(struct sk_buff *skb,
                newdst = htonl(0x7F000001);
        } else {
                struct in_device *indev;
-               struct in_ifaddr *ifa;
 
                newdst = 0;
 
                indev = __in_dev_get_rcu(skb->dev);
-               if (indev && indev->ifa_list) {
-                       ifa = indev->ifa_list;
-                       newdst = ifa->ifa_local;
+               if (indev) {
+                       struct in_ifaddr *ifa;
+
+                       ifa = READ_ONCE(indev->ifa_list); // rcu_dereference(xxx)
+
+                       if (ifa)
+                               newdst = ifa->ifa_local;
                }
 
                if (!newdst)

Re: [RFC] ifa_list needs proper rcu protection

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:

Sorry for late reply.

> It looks that unless RTNL is held, accessing ifa_list needs proper RCU protection ?
> 
> indev->ifa_list can be changed under us by another cpu (which owns RTNL)
> 
> Lets took an example.
> 
> (A proper rcu_dereference() with an happy sparse support would require adding __rcu attribute,
>  I put a READ_ONCE() which should be just fine in this particular context)

I don't see e.g. __inet_insert_ifa() use rcu_assign_pointer() or similar
primitive, so I don't think its enough to change readers.

Same for __inet_del_ifa(), i see freeing gets dealyed via call_rcu, but
it uses normal assignemts instead of a rcu helper.

So, I am afraid we will have to sprinkle some rcu_assign_/derefence in
several places.

Re: [RFC] ifa_list needs proper rcu protection

[Eric Dumazet]

On 5/4/19 2:01 PM, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> Sorry for late reply.
> 
>> It looks that unless RTNL is held, accessing ifa_list needs proper RCU protection ?
>>
>> indev->ifa_list can be changed under us by another cpu (which owns RTNL)
>>
>> Lets took an example.
>>
>> (A proper rcu_dereference() with an happy sparse support would require adding __rcu attribute,
>>  I put a READ_ONCE() which should be just fine in this particular context)
> 
> I don't see e.g. __inet_insert_ifa() use rcu_assign_pointer() or similar
> primitive, so I don't think its enough to change readers.
> 
> Same for __inet_del_ifa(), i see freeing gets dealyed via call_rcu, but
> it uses normal assignemts instead of a rcu helper.
> 
> So, I am afraid we will have to sprinkle some rcu_assign_/derefence in
> several places.

Yes, I came to the same conclusion.