| Message ID | 7bdc26e6-ce41-02ba-baef-3e4e908f6dd7@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] ifa_list needs proper rcu protection | expand |
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.
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.
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)