On 10/15/2015 08:01 AM, Matan Barak wrote: > When using ifup/ifdown while executing enum_netdev_ipv4_ips, > ifa could become invalid and cause use after free error. > Fixing it by protecting with RCU lock. > > Fixes: 03db3a2d81e6 ('IB/core: Add RoCE GID table management') > Signed-off-by: Matan Barak <matanb@xxxxxxxxxxxx> > --- > > Hi Doug, > > This patch fixes a bug in RoCE GID table implementation. Under stress conditions > where ifup/ifdown are used, the ifa pointer could become invalid. Using a > RCU lock in order to avoid freeing the ifa node (as done in other inet functions > (for example, inet_addr_onlink). > > Our QA team verified that this patch fixes this issue. This doesn't look like a good fix to me. In particular, I think you merely shifted the bug around, you didn't actually resolve it. In the original code, you called update_gid_ip while holding a reference to in_dev. The reference to in_dev was not enough to keep the ifa list from changing while you were doing your work. It's not surprising that you hit a race with the ifa list because update_gid_ip being called synchronously can both A) sleep because of the mutexes it takes and B) be slow because of how many locks it takes (and it can really take a lot due to find_gid) and C) be slow again because of updating the gid table calling into the low level driver and actually writing a mailbox command to the card. So, for all those reasons, not only did you hit this race, but you were *likely* to hit this race. Now, you've put an rcu_read_lock on ndev instead. And you're no longer seeing the race. However, does taking the rcu_read_lock on ndev actually protect the ifa list on ndev, or is the real fix the fact that you moved update_gid_ip out of the main loop? Before, you blocked while processing the ifa list, making hitting your race likely. Now you process the ifa list very fast and build your own sin_list that is no longer impacted by changes to the ifa list, but I don't know that the rcu_read_lock you have taken actually makes you for sure safe here versus the possibility that you have just made the race much harder to hit and hidden it. And even if the rcu_read_lock is for sure safe in terms of accessing the ifa list, these changes may have just introduced a totally new bug that your QE tests haven't exposed but might exist none the less. In particular, we have now queued up adding a bunch of gids to the ndev. But we drop our reference to the rcu lock, then we invoke a (possibly large number) of sleeping iterations. What's to prevent a situation where we get enum_netdev_ipv4_ips() called on say a vlan child interface of a primary RoCE netdev, create our address list, release our lock, then the user destroys our vlan device, and we race with del_netdev_ips on the vlan device, such that del_netdev_ips completes and removes all the gids for that netdev, but we still have backlogged gid add events in enum_netdev_ipv4_ips and so we add back in what will become permanently stale gids? I don't think we hold rtnl_lock while running in enum_netdev_ipv4_ips and that's probably the only lock that would exclude the user from deleting the vlan device, so as far as I can tell we can easily call del_netdev_ips while the tail end of enum_netdev_ipv4_ips is sleeping. Am I wrong here? A test would be to take whatever QE test you have that hit this bug in the first place, and on a different terminal add a while loop of adding/removing the same vlan interface that you are updating gids on and see if the gid table starts filling up with stale, unremovable entries. > Thanks, > Matan > > drivers/infiniband/core/roce_gid_mgmt.c | 35 +++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c > index 6b24cba..178f984 100644 > --- a/drivers/infiniband/core/roce_gid_mgmt.c > +++ b/drivers/infiniband/core/roce_gid_mgmt.c > @@ -250,25 +250,44 @@ static void enum_netdev_ipv4_ips(struct ib_device *ib_dev, > u8 port, struct net_device *ndev) > { > struct in_device *in_dev; > + struct sin_list { > + struct list_head list; > + struct sockaddr_in ip; > + }; > + struct sin_list *sin_iter; > + struct sin_list *sin_temp; > > + LIST_HEAD(sin_list); > if (ndev->reg_state >= NETREG_UNREGISTERING) > return; > > - in_dev = in_dev_get(ndev); > - if (!in_dev) > + rcu_read_lock(); > + in_dev = __in_dev_get_rcu(ndev); > + if (!in_dev) { > + rcu_read_unlock(); > return; > + } > > for_ifa(in_dev) { > - struct sockaddr_in ip; > + struct sin_list *entry = kzalloc(sizeof(*entry), GFP_ATOMIC); > > - ip.sin_family = AF_INET; > - ip.sin_addr.s_addr = ifa->ifa_address; > - update_gid_ip(GID_ADD, ib_dev, port, ndev, > - (struct sockaddr *)&ip); > + if (!entry) { > + pr_warn("roce_gid_mgmt: couldn't allocate entry for IPv4 update\n"); > + continue; > + } > + entry->ip.sin_family = AF_INET; > + entry->ip.sin_addr.s_addr = ifa->ifa_address; > + list_add_tail(&entry->list, &sin_list); > } > endfor_ifa(in_dev); > + rcu_read_unlock(); > > - in_dev_put(in_dev); > + list_for_each_entry_safe(sin_iter, sin_temp, &sin_list, list) { > + update_gid_ip(GID_ADD, ib_dev, port, ndev, > + (struct sockaddr *)&sin_iter->ip); > + list_del(&sin_iter->list); > + kfree(sin_iter); > + } > } > > static void enum_netdev_ipv6_ips(struct ib_device *ib_dev, > -- Doug Ledford <dledford@xxxxxxxxxx> GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature