* [PATCH] IB core: Fix locking on device numbers allocation
@ 2010-03-23 19:13 Eli Cohen
[not found] ` <20100323191317.GA14496-8YAHvHwT2UEvbXDkjdHOrw/a8Rv0c6iv@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Eli Cohen @ 2010-03-23 19:13 UTC (permalink / raw)
To: Roland Dreier; +Cc: Linux RDMA list
When the driver needs to dynamically allocate char device numbers in systems
with more than IB_UVERBS_MAX_DEVICES, it releases map lock, allocates a new
range and a new device number from that range, and only then re-acquires the
lock. This must be protected for the same reasoning that the map_lock spinlock
is used. Without protecting we could also end up calling alloc_chrdev_region()
a nubmer of times and cause a leakage. Fix this by replacing map_lock with a
mutex and apply on the all the allocation code.
Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
---
drivers/infiniband/core/uverbs_main.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index d805cf3..9589c71 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -72,7 +72,7 @@ DEFINE_IDR(ib_uverbs_cq_idr);
DEFINE_IDR(ib_uverbs_qp_idr);
DEFINE_IDR(ib_uverbs_srq_idr);
-static DEFINE_SPINLOCK(map_lock);
+static DEFINE_MUTEX(map_lock);
static DECLARE_BITMAP(dev_map, IB_UVERBS_MAX_DEVICES);
static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
@@ -738,15 +738,15 @@ static void ib_uverbs_add_one(struct ib_device *device)
kref_init(&uverbs_dev->ref);
init_completion(&uverbs_dev->comp);
- spin_lock(&map_lock);
+ mutex_lock(&map_lock);
devnum = find_first_zero_bit(dev_map, IB_UVERBS_MAX_DEVICES);
if (devnum >= IB_UVERBS_MAX_DEVICES) {
- spin_unlock(&map_lock);
devnum = find_overflow_devnum();
- if (devnum < 0)
+ if (devnum < 0) {
+ mutex_unlock(&map_lock);
goto err;
+ }
- spin_lock(&map_lock);
uverbs_dev->devnum = devnum + IB_UVERBS_MAX_DEVICES;
base = devnum + overflow_maj;
set_bit(devnum, overflow_map);
@@ -755,7 +755,7 @@ static void ib_uverbs_add_one(struct ib_device *device)
base = devnum + IB_UVERBS_BASE_DEV;
set_bit(devnum, dev_map);
}
- spin_unlock(&map_lock);
+ mutex_unlock(&map_lock);
uverbs_dev->ib_dev = device;
uverbs_dev->num_comp_vectors = device->num_comp_vectors;
--
1.7.0.3
--
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] IB core: Fix locking on device numbers allocation
[not found] ` <20100323191317.GA14496-8YAHvHwT2UEvbXDkjdHOrw/a8Rv0c6iv@public.gmane.org>
@ 2010-03-24 17:39 ` Roland Dreier
[not found] ` <adaiq8l1uwp.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Roland Dreier @ 2010-03-24 17:39 UTC (permalink / raw)
To: Eli Cohen; +Cc: Linux RDMA list, Alexander Chiang
> When the driver needs to dynamically allocate char device numbers in systems
> with more than IB_UVERBS_MAX_DEVICES, it releases map lock, allocates a new
> range and a new device number from that range, and only then re-acquires the
> lock. This must be protected for the same reasoning that the map_lock spinlock
> is used. Without protecting we could also end up calling alloc_chrdev_region()
> a nubmer of times and cause a leakage. Fix this by replacing map_lock with a
> mutex and apply on the all the allocation code.
Looks like a good catch. I assume you found this through inspection and
not hitting it practice? Also it seems user_mad.c would need the same fix.
Although looking at this I wonder if we do need that lock... we don't
seem to do any locking when we do the clear_bit in the dev_map, and all
of this is done through the device add/remove callback, which seems to
be serialized by the device_mutex in device.c. But we probably don't
want to make that a requirement in case we parallelize in the future.
- R.
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] IB core: Fix locking on device numbers allocation
[not found] ` <adaiq8l1uwp.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
@ 2010-03-25 7:36 ` Eli Cohen
[not found] ` <20100325073635.GF12224-8YAHvHwT2UEvbXDkjdHOrw/a8Rv0c6iv@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Eli Cohen @ 2010-03-25 7:36 UTC (permalink / raw)
To: Roland Dreier; +Cc: Eli Cohen, Linux RDMA list, Alexander Chiang
On Wed, Mar 24, 2010 at 10:39:18AM -0700, Roland Dreier wrote:
>
> Looks like a good catch. I assume you found this through inspection and
> not hitting it practice?
Correct, I caught this from inspecting the code.
> Also it seems user_mad.c would need the same fix.
Yes, I missed that.
>
> Although looking at this I wonder if we do need that lock... we don't
> seem to do any locking when we do the clear_bit in the dev_map, and all
> of this is done through the device add/remove callback, which seems to
> be serialized by the device_mutex in device.c. But we probably don't
> want to make that a requirement in case we parallelize in the future.
>
I missed the fact the clear_bit is not atomic. So to make this
complete I will send a new patch with protection on the clear bit.
Would you like me to send a patch for user_mad too or would you push
that?
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] IB core: Fix locking on device numbers allocation
[not found] ` <20100325073635.GF12224-8YAHvHwT2UEvbXDkjdHOrw/a8Rv0c6iv@public.gmane.org>
@ 2010-03-31 21:50 ` Roland Dreier
0 siblings, 0 replies; 4+ messages in thread
From: Roland Dreier @ 2010-03-31 21:50 UTC (permalink / raw)
To: Eli Cohen; +Cc: Eli Cohen, Linux RDMA list, Alexander Chiang
> I missed the fact the clear_bit is not atomic. So to make this
> complete I will send a new patch with protection on the clear bit.
> Would you like me to send a patch for user_mad too or would you push
> that?
Hmm, actually maybe clear_bit is atomic enough for us.
<asm-generic/atmoic.h> says:
* clear_bit() is atomic and may not be reordered. However, it does
* not contain a memory barrier, so if it is used for locking purposes,
* you should call smp_mb__before_clear_bit() and/or smp_mb__after_clear_bit()
* in order to ensure changes are visible on other processors.
and I don't think we have an issue with visibility of the updates -- the
worst case I guess is a tiny window where we fail to register a new
device just as we are unregistering another device on a different CPU.
I guess I knew that already once long ago, and so that's why there isn't
locking around the clear_bit parts of things.
- R.
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-31 21:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-23 19:13 [PATCH] IB core: Fix locking on device numbers allocation Eli Cohen
[not found] ` <20100323191317.GA14496-8YAHvHwT2UEvbXDkjdHOrw/a8Rv0c6iv@public.gmane.org>
2010-03-24 17:39 ` Roland Dreier
[not found] ` <adaiq8l1uwp.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
2010-03-25 7:36 ` Eli Cohen
[not found] ` <20100325073635.GF12224-8YAHvHwT2UEvbXDkjdHOrw/a8Rv0c6iv@public.gmane.org>
2010-03-31 21:50 ` Roland Dreier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.