[PATCH rdma-cm] IB/core: Fix memory corruption in ib_cache_gid_set_default_gid

[PATCH rdma-cm] IB/core: Fix memory corruption in ib_cache_gid_set_default_gid

[Matan Barak]

From: Doron Tsur <doront-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

When ib_cache_gid_set_default_gid is called from several threads,
updating the table could make find_gid fail, therefore a negative
index will be retruned and an invalid table entry will be used.
Locking find_gid as well fixes this problem.

Fixes: 03db3a2d81e6 ('IB/core: Add RoCE GID table management')
Signed-off-by: Doron Tsur <doront-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---

Hi Doug,

This patch fixes a bug in RoCE GID table implementation. When several
instances executes ib_cache_gid_set_default_gid, we could try to update
the same default GID (at the same index) simultaneously.
Therefore, find_gid will fail finding this default GID and we'll hit the
WARN_ON condition.

We hit this bug while testing this code under pressure of doing ifup/ifdown.

Thanks,
Matan

 drivers/infiniband/core/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 8f66c67..87471ef 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -508,12 +508,12 @@ void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port,
 	memset(&gid_attr, 0, sizeof(gid_attr));
 	gid_attr.ndev = ndev;
 
+	mutex_lock(&table->lock);
 	ix = find_gid(table, NULL, NULL, true, GID_ATTR_FIND_MASK_DEFAULT);
 
 	/* Coudn't find default GID location */
 	WARN_ON(ix < 0);
 
-	mutex_lock(&table->lock);
 	if (!__ib_cache_gid_get(ib_dev, port, ix,
 				&current_gid, &current_gid_attr) &&
 	    mode == IB_CACHE_GID_DEFAULT_MODE_SET &&
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-cm] IB/core: Fix use after free of ifa

[Matan Barak]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---

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.

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,
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 5729 bytes --]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
> 
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Jason Gunthorpe]

On Thu, Oct 15, 2015 at 01:37:22PM -0400, Doug Ledford wrote:

> 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

It looks OK, the free side for using call_rcu - though I'm not quite
sure why for_ifa isn't using rcu_dereference...

> 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.

This event driven design is super complex/frail, if it actually
doesn't work, it should probably be changed to a level based system.

- Record incoming events indicating a change in ip list has happened
  for an ib device, Schedule a task to refresh the list
- Refresh task holds rtnl locks and builds a list of IP addresses associated
  with the ib device. This is atomic with user changes to the IP list.
- Drops locks and then directly synchs the discovered list above with
  the device's record of associated IPs. (ie run set intersection)

It is much less subtle than hoping all the async event driven stuff
in here works out, probably a lot less code too.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Matan Barak]

On Thu, Oct 15, 2015 at 8:53 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Thu, Oct 15, 2015 at 01:37:22PM -0400, Doug Ledford wrote:
>
>> 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
>
> It looks OK, the free side for using call_rcu - though I'm not quite
> sure why for_ifa isn't using rcu_dereference...
>

I was wondering the same regarding the rcu_dereference.

>> 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.
>
> This event driven design is super complex/frail, if it actually
> doesn't work, it should probably be changed to a level based system.
>
> - Record incoming events indicating a change in ip list has happened
>   for an ib device, Schedule a task to refresh the list
> - Refresh task holds rtnl locks and builds a list of IP addresses associated
>   with the ib device. This is atomic with user changes to the IP list.
> - Drops locks and then directly synchs the discovered list above with
>   the device's record of associated IPs. (ie run set intersection)
>
> It is much less subtle than hoping all the async event driven stuff
> in here works out, probably a lot less code too.
>

It's simpler, but every IP change could make us refresh the whole list
and possibly the GID indices as well.

> Jason
> --

Matan

> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Jason Gunthorpe]

On Sun, Oct 18, 2015 at 10:51:04AM +0300, Matan Barak wrote:

> It's simpler, but every IP change could make us refresh the whole list
> and possibly the GID indices as well.

No, it is trivial to use a set intersection algorithm to convert two
full current state lists into a sequence of add/remove's for the
driver and preserve the indices.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Matan Barak]

On Thu, Oct 15, 2015 at 8:37 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>>
>> 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.
>

I don't mind that the list could be changing between the inet event
and the work handler.
I do mind the ifa is released while working on it. I think the major
reason for possible slowness is the vendor call. Most locks are
per-entry and are read-write locks.

> 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.
>

As Jason wrote, the release of the ifa is protected by call_rcu. So
protecting the usage of ifa with RCU should be enough to eliminate
this bug.


> 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.
>

The RoCE GID management design uses events handlers and one workqueue.
When an event (inet/net) is handled, we hold the net device and
execute a work in the workqueue.
The works are executed in a queue - thus first-come first-served.
That's why if you add/del a vlan (or its IP)
we do dev_hold in the event itself. Since the ndev is available in the
event and is held when executing the event - it can't be deleted until
we handle this
event in the workqueue. If the user tries to delete the vlan before
our add (inet/ndev) work was completed, we'll get an UNREGISTER event,
but since the dev is held, the stack will have to wait until we free
all our ref counts to this device. Using a queue guarantees us the
order - we'll first complete adding the vlan and then delete it. Only
after all reference counts are dropped, the net device could be
deleted.
Anyway, I'll ask the QA team here to add this test.

Thanks for taking a look on this patch.

>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 9906 bytes --]

On 10/18/2015 03:49 AM, Matan Barak wrote:
> On Thu, Oct 15, 2015 at 8:37 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> ---
>>>
>>> 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.
>>
> 
> I don't mind that the list could be changing between the inet event
> and the work handler.
> I do mind the ifa is released while working on it. I think the major
> reason for possible slowness is the vendor call.

No, it's not.

> Most locks are
> per-entry and are read-write locks.

This is a major cause of the slowness.  Unless you have a specific need
of them, per-entry rwlocks are *NOT* a good idea.  I was going to bring
this up separately, so I'll just mention it here.  Per entry locks help
reduce contention when you have lots of multiple accessors.  Using
rwlocks help reduce contention when you have a read-mostly entry that is
only occasionally changed.  But every lock and every unlock (just like
every atomic access) still requires a locked memory cycle.  That means
every lock acquire and every lock release requires a synchronization
event between all CPUs.  Using per-entry locks means that every entry
triggers two of those synchronization events.  On modern Intel CPUs,
they cost about 32 cycles per event.  If you are going to do something,
and it can't be done without a lock, then grab a single lock and do it
as fast as you can.  Only in rare cases would you want per-entry locks.

>> 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.
>>
> 
> As Jason wrote, the release of the ifa is protected by call_rcu. So
> protecting the usage of ifa with RCU should be enough to eliminate
> this bug.

OK, I'm happy enough with the explanation to take this patch.  But
please think about the per-entry locks you mention above.  Those need to
go if possible.  I'm relatively certain that you will be able to
demonstrate a *drastic* speed up in this code with them removed (try
running the cmtime application from librdmacm-utils and see what the
results are with per-entry locks and a per table lock instead).

> 
>> 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.
>>
> 
> The RoCE GID management design uses events handlers and one workqueue.
> When an event (inet/net) is handled, we hold the net device and
> execute a work in the workqueue.
> The works are executed in a queue - thus first-come first-served.
> That's why if you add/del a vlan (or its IP)
> we do dev_hold in the event itself. Since the ndev is available in the
> event and is held when executing the event - it can't be deleted until
> we handle this
> event in the workqueue. If the user tries to delete the vlan before
> our add (inet/ndev) work was completed, we'll get an UNREGISTER event,
> but since the dev is held, the stack will have to wait until we free
> all our ref counts to this device. Using a queue guarantees us the
> order - we'll first complete adding the vlan and then delete it. Only
> after all reference counts are dropped, the net device could be
> deleted.
> Anyway, I'll ask the QA team here to add this test.

OK.  And it works for now, but if there is ever added an additional
means of running one of these functions that isn't on the work queue,
then it can break subtly.

> Thanks for taking a look on this patch.
> 
>>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>               GPG KeyID: 0E572FDD
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Matan Barak]

On 10/19/2015 3:23 PM, Doug Ledford wrote:
> On 10/18/2015 03:49 AM, Matan Barak wrote:
>> On Thu, Oct 15, 2015 at 8:37 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>
>>>> 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.
>>>
>>
>> I don't mind that the list could be changing between the inet event
>> and the work handler.
>> I do mind the ifa is released while working on it. I think the major
>> reason for possible slowness is the vendor call.
>
> No, it's not.
>
>> Most locks are
>> per-entry and are read-write locks.
>
> This is a major cause of the slowness.  Unless you have a specific need
> of them, per-entry rwlocks are *NOT* a good idea.  I was going to bring
> this up separately, so I'll just mention it here.  Per entry locks help
> reduce contention when you have lots of multiple accessors.  Using
> rwlocks help reduce contention when you have a read-mostly entry that is
> only occasionally changed.  But every lock and every unlock (just like
> every atomic access) still requires a locked memory cycle.  That means
> every lock acquire and every lock release requires a synchronization
> event between all CPUs.  Using per-entry locks means that every entry
> triggers two of those synchronization events.  On modern Intel CPUs,
> they cost about 32 cycles per event.  If you are going to do something,
> and it can't be done without a lock, then grab a single lock and do it
> as fast as you can.  Only in rare cases would you want per-entry locks.
>

I agree that every rwlock costs us locked access. However, lets look at 
the common scenario here. I think that in production stable systems, the 
IPs our rdma-cm stack uses (and thus GIDs) should be pretty stable. 
Thus, most accesses should be read calls. That's why IMHO read-write 
access makes sense here.

Regarding single-lock vs per entry lock, it really depends on common an 
entry could be updated while another entry is being used by an 
application. In a (future) dynamic system, you might want to create 
containers dynamically, which will add a net device and change the 
hardware GID table while other application (maybe in another container) 
uses other GIDs, but this might be rare scenario.

>>> 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.
>>>
>>
>> As Jason wrote, the release of the ifa is protected by call_rcu. So
>> protecting the usage of ifa with RCU should be enough to eliminate
>> this bug.
>
> OK, I'm happy enough with the explanation to take this patch.  But
> please think about the per-entry locks you mention above.  Those need to
> go if possible.  I'm relatively certain that you will be able to
> demonstrate a *drastic* speed up in this code with them removed (try
> running the cmtime application from librdmacm-utils and see what the
> results are with per-entry locks and a per table lock instead).
>

Ok, refactoring this code for a single lock shouldn't be problematic. 
Regarding performance, I think the results here are vastly impacted by 
the rate we'll add/remove IPs or upper net-devices in the background.

>>
>>> 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.
>>>
>>
>> The RoCE GID management design uses events handlers and one workqueue.
>> When an event (inet/net) is handled, we hold the net device and
>> execute a work in the workqueue.
>> The works are executed in a queue - thus first-come first-served.
>> That's why if you add/del a vlan (or its IP)
>> we do dev_hold in the event itself. Since the ndev is available in the
>> event and is held when executing the event - it can't be deleted until
>> we handle this
>> event in the workqueue. If the user tries to delete the vlan before
>> our add (inet/ndev) work was completed, we'll get an UNREGISTER event,
>> but since the dev is held, the stack will have to wait until we free
>> all our ref counts to this device. Using a queue guarantees us the
>> order - we'll first complete adding the vlan and then delete it. Only
>> after all reference counts are dropped, the net device could be
>> deleted.
>> Anyway, I'll ask the QA team here to add this test.
>
> OK.  And it works for now, but if there is ever added an additional
> means of running one of these functions that isn't on the work queue,
> then it can break subtly.
>

This workqueue is an important part of this design. We need to impose an 
order, but handle events in a different context (as some IPv6 events 
come from an atomic context).

>> Thanks for taking a look on this patch.
>>
>>>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>                GPG KeyID: 0E572FDD
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 5991 bytes --]

On 10/19/2015 10:20 AM, Matan Barak wrote:
> On 10/19/2015 3:23 PM, Doug Ledford wrote:

>> This is a major cause of the slowness.  Unless you have a specific need
>> of them, per-entry rwlocks are *NOT* a good idea.  I was going to bring
>> this up separately, so I'll just mention it here.  Per entry locks help
>> reduce contention when you have lots of multiple accessors.  Using
>> rwlocks help reduce contention when you have a read-mostly entry that is
>> only occasionally changed.  But every lock and every unlock (just like
>> every atomic access) still requires a locked memory cycle.  That means
>> every lock acquire and every lock release requires a synchronization
>> event between all CPUs.  Using per-entry locks means that every entry
>> triggers two of those synchronization events.  On modern Intel CPUs,
>> they cost about 32 cycles per event.  If you are going to do something,
>> and it can't be done without a lock, then grab a single lock and do it
>> as fast as you can.  Only in rare cases would you want per-entry locks.
>>
> 
> I agree that every rwlock costs us locked access. However, lets look at
> the common scenario here.

No, let's not.  Especially if your "common scenario" causes you to
ignore pathological bad behavior.  Optimizing for the common case is not
an excuse to enable pathological behavior.

> I think that in production stable systems, the
> IPs our rdma-cm stack uses (and thus GIDs) should be pretty stable.

Probably true.

> Thus, most accesses should be read calls.

Probably true.

> That's why IMHO read-write
> access makes sense here.

Probably true.

> Regarding single-lock vs per entry lock, it really depends on common an
> entry could be updated while another entry is being used by an
> application.

No, it depends on more than that.  In addition to the frequency of
updates versus lookups, there is also the issue of lookup cost and
update cost.  Using per entry locks makes both the lookup cost and the
update cost of anything other than the first gid in the table grow
exponentially.  A good way to demonstrate this would be to create 100
vlans on a RoCE device, then run the cmtime test between two hosts using
the 100th vlan IP at each end.  Using per-entry locks, the performance
of this test relative to using the first IP on the device will be
pathologically bad.

> In a (future) dynamic system, you might want to create
> containers dynamically, which will add a net device and change the
> hardware GID table while other application (maybe in another container)
> uses other GIDs, but this might be rare scenario.

This is a non-issue.  Creating a container and an application inside
that container to use an RDMA interface is already a heavy weight
operation (minimum fork/exec/program startup).  Table lookups can be
needed thousands of times per second under certain conditions and must
be fast.  Per-entry locks are only fast for the first item in the list.
 After that, they progressively get slower and slower than usage with a
single lock.

> Ok, refactoring this code for a single lock shouldn't be problematic.
> Regarding performance, I think the results here are vastly impacted by
> the rate we'll add/remove IPs or upper net-devices in the background.

Not really.  You will never add/remove IPs fast enough to justify
per-entry locks, especially when you consider that ib_cache_gid_add
calls find_gid twice, once going over the entire table if the gid isn't
found, before ever calling add_gid.  The ocrdma table isn't bad because
it's only 16 entries.  But the mlx4 table is going to be 128 I suspect,
so you can do the test I mentioned above and use 100 vlans.  In that
scenario, the 101st gid will require that you take and release 128 locks
to scan for the entry in the list, it will come back empty, then you
will take and release 101 locks until you find an empty entry, and then
you will take a write lock as you write the gid.  That's 230 total
locks, for 460 total locked memory operations at 32 cycles each, so a
total of 14,720 cycles spent doing nothing but locking the memory bus.
And each of those operations slows down all of the CPUs in the system.

Where I've seen this sort of behavior totally wreck a production system
is when the GID you need to look up is on port 2.  Even if it is the
firstmost GID on port 2, if you have to search all of port 1's table
first, then by the time you get to port 2's table, you've already lost.

So, here's some suggestions:

1)  Use a single lock on the table per port.
2)  Change find_gid so that it will take an additional element, int
*empty_idx, and as it is scanning the gid table, when it runs across an
empty entry, if empty_idx != NULL, *empty_idx = idx;.  This prevents
needing to scan the array twice.
3)  Consider optimizing the code by requiring that any time a GID is
added, it must take the first empty spot, and any time one is deleted,
any valid entries after it must be moved up to fill the empty spot, then
optimize find_gid to quit once it finds an empty slot because it knows
the rest of the table is empty.  Given that mlx4 keeps a huge 128 table
array, this can be very helpful.
4)  Does the table port struct have to use a mutex?  Could it be changed
to a rwlock instead?  If so, you could changing write_gid so that it
assumes it is called with the read lock on the port already held and it
merely updates itself to a write lock when it is ready to write the
entry to the array and update the card.  It would then downgrade back to
a read lock on return and the calling function would hold the single
read lock from the time it is ready to call find_gid until write_gid has
returned.
5)  I noticed that ib_cache_gid_find_by_port calls find_gid without even
taking the read lock, that needs fixed.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Matan Barak]

On 10/19/2015 6:27 PM, Doug Ledford wrote:
> On 10/19/2015 10:20 AM, Matan Barak wrote:
>> On 10/19/2015 3:23 PM, Doug Ledford wrote:
>
>>> This is a major cause of the slowness.  Unless you have a specific need
>>> of them, per-entry rwlocks are *NOT* a good idea.  I was going to bring
>>> this up separately, so I'll just mention it here.  Per entry locks help
>>> reduce contention when you have lots of multiple accessors.  Using
>>> rwlocks help reduce contention when you have a read-mostly entry that is
>>> only occasionally changed.  But every lock and every unlock (just like
>>> every atomic access) still requires a locked memory cycle.  That means
>>> every lock acquire and every lock release requires a synchronization
>>> event between all CPUs.  Using per-entry locks means that every entry
>>> triggers two of those synchronization events.  On modern Intel CPUs,
>>> they cost about 32 cycles per event.  If you are going to do something,
>>> and it can't be done without a lock, then grab a single lock and do it
>>> as fast as you can.  Only in rare cases would you want per-entry locks.
>>>
>>
>> I agree that every rwlock costs us locked access. However, lets look at
>> the common scenario here.
>
> No, let's not.  Especially if your "common scenario" causes you to
> ignore pathological bad behavior.  Optimizing for the common case is not
> an excuse to enable pathological behavior.
>
>> I think that in production stable systems, the
>> IPs our rdma-cm stack uses (and thus GIDs) should be pretty stable.
>
> Probably true.
>
>> Thus, most accesses should be read calls.
>
> Probably true.
>
>> That's why IMHO read-write
>> access makes sense here.
>
> Probably true.
>
>> Regarding single-lock vs per entry lock, it really depends on common an
>> entry could be updated while another entry is being used by an
>> application.
>
> No, it depends on more than that.  In addition to the frequency of
> updates versus lookups, there is also the issue of lookup cost and
> update cost.  Using per entry locks makes both the lookup cost and the
> update cost of anything other than the first gid in the table grow
> exponentially.  A good way to demonstrate this would be to create 100
> vlans on a RoCE device, then run the cmtime test between two hosts using
> the 100th vlan IP at each end.  Using per-entry locks, the performance
> of this test relative to using the first IP on the device will be
> pathologically bad.
>

That's correct, we"waste" #entries * <time of lock and unlock>. But we 
have to note this is mainly during the connecting part.

>> In a (future) dynamic system, you might want to create
>> containers dynamically, which will add a net device and change the
>> hardware GID table while other application (maybe in another container)
>> uses other GIDs, but this might be rare scenario.
>
> This is a non-issue.  Creating a container and an application inside
> that container to use an RDMA interface is already a heavy weight
> operation (minimum fork/exec/program startup).  Table lookups can be
> needed thousands of times per second under certain conditions and must
> be fast.  Per-entry locks are only fast for the first item in the list.
>   After that, they progressively get slower and slower than usage with a
> single lock.
>

We first used RCU and seqcount in order to protect each entry. When we 
changed that to rwlock, we added some per-entry work.

>> Ok, refactoring this code for a single lock shouldn't be problematic.
>> Regarding performance, I think the results here are vastly impacted by
>> the rate we'll add/remove IPs or upper net-devices in the background.
>
> Not really.  You will never add/remove IPs fast enough to justify
> per-entry locks, especially when you consider that ib_cache_gid_add
> calls find_gid twice, once going over the entire table if the gid isn't
> found, before ever calling add_gid.  The ocrdma table isn't bad because
> it's only 16 entries.  But the mlx4 table is going to be 128 I suspect,
> so you can do the test I mentioned above and use 100 vlans.  In that
> scenario, the 101st gid will require that you take and release 128 locks
> to scan for the entry in the list, it will come back empty, then you
> will take and release 101 locks until you find an empty entry, and then
> you will take a write lock as you write the gid.  That's 230 total
> locks, for 460 total locked memory operations at 32 cycles each, so a
> total of 14,720 cycles spent doing nothing but locking the memory bus.
> And each of those operations slows down all of the CPUs in the system.
>
> Where I've seen this sort of behavior totally wreck a production system
> is when the GID you need to look up is on port 2.  Even if it is the
> firstmost GID on port 2, if you have to search all of port 1's table
> first, then by the time you get to port 2's table, you've already lost.
>
> So, here's some suggestions:
>
> 1)  Use a single lock on the table per port.

Make sense as an enhancement.

> 2)  Change find_gid so that it will take an additional element, int
> *empty_idx, and as it is scanning the gid table, when it runs across an
> empty entry, if empty_idx != NULL, *empty_idx = idx;.  This prevents
> needing to scan the array twice.

Make sense as an enhancement.

> 3)  Consider optimizing the code by requiring that any time a GID is
> added, it must take the first empty spot, and any time one is deleted,
> any valid entries after it must be moved up to fill the empty spot, then
> optimize find_gid to quit once it finds an empty slot because it knows
> the rest of the table is empty.  Given that mlx4 keeps a huge 128 table
> array, this can be very helpful.

I don't think we want to change existing GID entries. Entities sometimes 
want to refer to GIDs via GID indices. Changing the GIDs order under 
their feet could be problematic.

> 4)  Does the table port struct have to use a mutex?  Could it be changed
> to a rwlock instead?  If so, you could changing write_gid so that it
> assumes it is called with the read lock on the port already held and it
> merely updates itself to a write lock when it is ready to write the
> entry to the array and update the card.  It would then downgrade back to
> a read lock on return and the calling function would hold the single
> read lock from the time it is ready to call find_gid until write_gid has
> returned.

AFAIK, read_locks can't be "upgraded" to write_locks. In addition, the 
cost of the locks here is negligible (assuming it's a per-table rwlock 
and not per entry lock). In all code paths that uses the table mutex, we 
call the vendor's command to write the GID table. Updating the GID table 
usually cost a lot more than the mutex locks.

> 5)  I noticed that ib_cache_gid_find_by_port calls find_gid without even
> taking the read lock, that needs fixed.
>
>

find_gid takes a per-entry read_lock. Which lock do you think is missing 
here?

Although it's mostly used in the connection part, I think the locking 
refactoring here is a good performance enhancement.
Because it is an enhancement, do you agree we could work here by gradual 
changes and change the locking schema later?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 7270 bytes --]

On 10/20/2015 10:50 AM, Matan Barak wrote:
> 
> 
> On 10/19/2015 6:27 PM, Doug Ledford wrote:
>> On 10/19/2015 10:20 AM, Matan Barak wrote:
>>> On 10/19/2015 3:23 PM, Doug Ledford wrote:
>>
>>>> This is a major cause of the slowness.  Unless you have a specific need
>>>> of them, per-entry rwlocks are *NOT* a good idea.  I was going to bring
>>>> this up separately, so I'll just mention it here.  Per entry locks help
>>>> reduce contention when you have lots of multiple accessors.  Using
>>>> rwlocks help reduce contention when you have a read-mostly entry
>>>> that is
>>>> only occasionally changed.  But every lock and every unlock (just like
>>>> every atomic access) still requires a locked memory cycle.  That means
>>>> every lock acquire and every lock release requires a synchronization
>>>> event between all CPUs.  Using per-entry locks means that every entry
>>>> triggers two of those synchronization events.  On modern Intel CPUs,
>>>> they cost about 32 cycles per event.  If you are going to do something,
>>>> and it can't be done without a lock, then grab a single lock and do it
>>>> as fast as you can.  Only in rare cases would you want per-entry locks.
>>>>
>>>
>>> I agree that every rwlock costs us locked access. However, lets look at
>>> the common scenario here.
>>
>> No, let's not.  Especially if your "common scenario" causes you to
>> ignore pathological bad behavior.  Optimizing for the common case is not
>> an excuse to enable pathological behavior.
>>
>>> I think that in production stable systems, the
>>> IPs our rdma-cm stack uses (and thus GIDs) should be pretty stable.
>>
>> Probably true.
>>
>>> Thus, most accesses should be read calls.
>>
>> Probably true.
>>
>>> That's why IMHO read-write
>>> access makes sense here.
>>
>> Probably true.
>>
>>> Regarding single-lock vs per entry lock, it really depends on common an
>>> entry could be updated while another entry is being used by an
>>> application.
>>
>> No, it depends on more than that.  In addition to the frequency of
>> updates versus lookups, there is also the issue of lookup cost and
>> update cost.  Using per entry locks makes both the lookup cost and the
>> update cost of anything other than the first gid in the table grow
>> exponentially.  A good way to demonstrate this would be to create 100
>> vlans on a RoCE device, then run the cmtime test between two hosts using
>> the 100th vlan IP at each end.  Using per-entry locks, the performance
>> of this test relative to using the first IP on the device will be
>> pathologically bad.
>>
> 
> That's correct, we"waste" #entries * <time of lock and unlock>. But we
> have to note this is mainly during the connecting part.

Yes, and I spent a *month* of my life tracking down why a customer's
real world application was failing in production (but not in their
staging environment) and it all boiled down to the fact that when their
app had to start up more than about 700-900 connections at app startup
and their setup included dual port controllers with both ports in use,
and some of those connections had to look up GIDs on the second port,
the abysmal per-gid lookup rate meant that the kernel had a hard ceiling
of how many GIDs it could look up per second.  Once that was hit, the
app went into a permanent cycle of dropping failed connections and
re-opening new ones and it would simply never catch up.

So it concerns me greatly to hear you say "But we have to note this is
mainly during the connecting part", implying that some wasted cycles on
connection startup are OK.  They aren't.  Not ever.  Production systems
fall over dead precisely because of things like this.

>> 3)  Consider optimizing the code by requiring that any time a GID is
>> added, it must take the first empty spot, and any time one is deleted,
>> any valid entries after it must be moved up to fill the empty spot, then
>> optimize find_gid to quit once it finds an empty slot because it knows
>> the rest of the table is empty.  Given that mlx4 keeps a huge 128 table
>> array, this can be very helpful.
> 
> I don't think we want to change existing GID entries. Entities sometimes
> want to refer to GIDs via GID indices. Changing the GIDs order under
> their feet could be problematic.

I had a feeling this one might be an issue.  There is, however, a
solution that would resolve the problem.  The issue here is that you
have an array of GIDs that you want to be able to both scan efficiently
and access directly.  The array is great for direct access, but the scan
efficiently part is a total looser because you scan so many empty
entries most of the time.  You could change it so that a gid port table
is actually a combination of 1) an array of gid_port_table_element
pointers and 2) a linked list of gid_port_table_element structs.  Then
you only allocate a struct when needed, link it into the list, assign it
an index, set the pointer in the array at point index to point to your
struct, and that way you can still direct access the elements, but can
also scan efficiently.  It's more complex, but would speed up the
scanning considerably.

>> 4)  Does the table port struct have to use a mutex?  Could it be changed
>> to a rwlock instead?  If so, you could changing write_gid so that it
>> assumes it is called with the read lock on the port already held and it
>> merely updates itself to a write lock when it is ready to write the
>> entry to the array and update the card.  It would then downgrade back to
>> a read lock on return and the calling function would hold the single
>> read lock from the time it is ready to call find_gid until write_gid has
>> returned.
> 
> AFAIK, read_locks can't be "upgraded" to write_locks. In addition, the
> cost of the locks here is negligible (assuming it's a per-table rwlock
> and not per entry lock). In all code paths that uses the table mutex, we
> call the vendor's command to write the GID table. Updating the GID table
> usually cost a lot more than the mutex locks.

I thought the kernel allowed upgrades, but I admit I didn't check.
Anyway, it was merely to prevent the case where you drop the read lock
and try to take the write lock and other readers rush in.  You could
also take the write lock from the beginning, but that might hurt
concurrency.

>> 5)  I noticed that ib_cache_gid_find_by_port calls find_gid without even
>> taking the read lock, that needs fixed.
> 
> find_gid takes a per-entry read_lock. Which lock do you think is missing
> here?

The other places hold the table mutex.

> Although it's mostly used in the connection part,

Please don't say that as though it makes the inefficiencies in this code
OK.  It doesn't.  Being on the connection startup or teardown path is
not an excuse.

> I think the locking
> refactoring here is a good performance enhancement.
> Because it is an enhancement, do you agree we could work here by gradual
> changes and change the locking schema later?

I never intended it to be during the rc phase.  But it would be good to
tackle soon.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This is in my tree for -rc.  Thanks.

> ---
> 
> 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.
> 
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-cm] IB/core: Fix use after free of ifa

[Matan Barak]

On Tue, Oct 20, 2015 at 10:17 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> This is in my tree for -rc.  Thanks.
>

The series "[PATCH for-next 0/3] Change per-entry locks in GID cache
to table lock" I've posted a while ago addresses the locking concerns.
Thanks.

>> ---
>>
>> 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.
>>
>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-cm] IB/core: Fix memory corruption in ib_cache_gid_set_default_gid

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On 10/15/2015 08:01 AM, Matan Barak wrote:
> From: Doron Tsur <doront-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> When ib_cache_gid_set_default_gid is called from several threads,
> updating the table could make find_gid fail, therefore a negative
> index will be retruned and an invalid table entry will be used.
> Locking find_gid as well fixes this problem.
> 
> Fixes: 03db3a2d81e6 ('IB/core: Add RoCE GID table management')
> Signed-off-by: Doron Tsur <doront-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
> 
> Hi Doug,
> 
> This patch fixes a bug in RoCE GID table implementation. When several
> instances executes ib_cache_gid_set_default_gid, we could try to update
> the same default GID (at the same index) simultaneously.
> Therefore, find_gid will fail finding this default GID and we'll hit the
> WARN_ON condition.
> 
> We hit this bug while testing this code under pressure of doing ifup/ifdown.
> 
> Thanks,
> Matan

Safe enough, applied for rc.

> 
>  drivers/infiniband/core/cache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 8f66c67..87471ef 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -508,12 +508,12 @@ void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port,
>  	memset(&gid_attr, 0, sizeof(gid_attr));
>  	gid_attr.ndev = ndev;
>  
> +	mutex_lock(&table->lock);
>  	ix = find_gid(table, NULL, NULL, true, GID_ATTR_FIND_MASK_DEFAULT);
>  
>  	/* Coudn't find default GID location */
>  	WARN_ON(ix < 0);
>  
> -	mutex_lock(&table->lock);
>  	if (!__ib_cache_gid_get(ib_dev, port, ix,
>  				&current_gid, &current_gid_attr) &&
>  	    mode == IB_CACHE_GID_DEFAULT_MODE_SET &&
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]