[PATCH] idr: Don't WARN in idr_find

[PATCH] idr: Don't WARN in idr_find

[Peter Hurley]

Since idr is used for syscall apis (to associate 'handles' with
internal structures), don't WARN with invalid input.

For example, POSIX timers are identified by timer_t id. These
ids are idr values. If userspace passes a representable timer_t id
value (eg, id < 0) but which was not previous allocated (since the
current idr api does not return negative idr values), then the
syscall properly returns an error; a WARN is unnecessary and
undesirable.

[  188.957053] WARNING: at /home/peter/src/kernels/next/lib/idr.c:669 idr_find_slowpath+0x151/0x170()
[  188.957055] Hardware name: Bochs
[  188.957105] Modules linked in: nfnetlink scsi_transport_iscsi can_raw ipt_ULOG
<...snip...>
[  188.957109] Pid: 2682, comm: trinity-child2 Not tainted 3.9.0-next-20130220+ldsem-xeon+lockdep #20130220+ldsem
[  188.957112] Call Trace:
[  188.957124]  [<ffffffff8105ecdf>] warn_slowpath_common+0x7f/0xc0
[  188.957126]  [<ffffffff8105ed3a>] warn_slowpath_null+0x1a/0x20
[  188.957129]  [<ffffffff813a8691>] idr_find_slowpath+0x151/0x170
[  188.957139]  [<ffffffff8108d787>] __lock_timer+0x97/0x2b0
[  188.957142]  [<ffffffff8108d6f5>] ? __lock_timer+0x5/0x2b0
[  188.957145]  [<ffffffff8108e0b7>] sys_timer_getoverrun+0x17/0x50
[  188.957152]  [<ffffffff817aeb99>] system_call_fastpath+0x16/0x1b
[  188.957156] ---[ end trace 25fe46b21eb9d42d ]---

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 lib/idr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/idr.c b/lib/idr.c
index 1a30272..8e9b1ac 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -666,7 +666,7 @@ void *idr_find_slowpath(struct idr *idp, int id)
 	int n;
 	struct idr_layer *p;
 
-	if (WARN_ON_ONCE(id < 0))
+	if (id < 0)
 		return NULL;
 
 	p = rcu_dereference_raw(idp->top);
-- 
1.8.1.2

Re: [PATCH] idr: Don't WARN in idr_find

[Tejun Heo]

On Mon, Feb 25, 2013 at 08:37:12PM -0500, Peter Hurley wrote:
> Since idr is used for syscall apis (to associate 'handles' with
> internal structures), don't WARN with invalid input.
> 
> For example, POSIX timers are identified by timer_t id. These
> ids are idr values. If userspace passes a representable timer_t id
> value (eg, id < 0) but which was not previous allocated (since the
> current idr api does not return negative idr values), then the
> syscall properly returns an error; a WARN is unnecessary and
> undesirable.

WARN_ON() on negative was intentional.  Because the previous
implmentation silently dropped the msb, we at least wanna know who has
been passing in negative indices.  Patch for the lock_timer is already
in -mm.

Thanks.

-- 
tejun

Re: [PATCH] idr: Don't WARN in idr_find

[Peter Hurley]

On Mon, 2013-02-25 at 17:40 -0800, Tejun Heo wrote:
> On Mon, Feb 25, 2013 at 08:37:12PM -0500, Peter Hurley wrote:
> > Since idr is used for syscall apis (to associate 'handles' with
> > internal structures), don't WARN with invalid input.
> > 
> > For example, POSIX timers are identified by timer_t id. These
> > ids are idr values. If userspace passes a representable timer_t id
> > value (eg, id < 0) but which was not previous allocated (since the
> > current idr api does not return negative idr values), then the
> > syscall properly returns an error; a WARN is unnecessary and
> > undesirable.
> 
> WARN_ON() on negative was intentional.  Because the previous
> implmentation silently dropped the msb, we at least wanna know who has
> been passing in negative indices.  Patch for the lock_timer is already
> in -mm.

Thanks. Can you share the patch title? It's not in next-20130225.

Peter

Re: [PATCH] idr: Don't WARN in idr_find

[Tejun Heo]

On Mon, Feb 25, 2013 at 08:46:11PM -0500, Peter Hurley wrote:
> On Mon, 2013-02-25 at 17:40 -0800, Tejun Heo wrote:
> > On Mon, Feb 25, 2013 at 08:37:12PM -0500, Peter Hurley wrote:
> > > Since idr is used for syscall apis (to associate 'handles' with
> > > internal structures), don't WARN with invalid input.
> > > 
> > > For example, POSIX timers are identified by timer_t id. These
> > > ids are idr values. If userspace passes a representable timer_t id
> > > value (eg, id < 0) but which was not previous allocated (since the
> > > current idr api does not return negative idr values), then the
> > > syscall properly returns an error; a WARN is unnecessary and
> > > undesirable.
> > 
> > WARN_ON() on negative was intentional.  Because the previous
> > implmentation silently dropped the msb, we at least wanna know who has
> > been passing in negative indices.  Patch for the lock_timer is already
> > in -mm.
> 
> Thanks. Can you share the patch title? It's not in next-20130225.

It's this one.

 http://thread.gmane.org/gmane.linux.kernel/1444883/focus=1445094

Thanks.

-- 
tejun

Re: [PATCH] idr: Don't WARN in idr_find

[Peter Hurley]

On Mon, 2013-02-25 at 17:48 -0800, Tejun Heo wrote:
> On Mon, Feb 25, 2013 at 08:46:11PM -0500, Peter Hurley wrote:
> > On Mon, 2013-02-25 at 17:40 -0800, Tejun Heo wrote:
> > > On Mon, Feb 25, 2013 at 08:37:12PM -0500, Peter Hurley wrote:
> > > > Since idr is used for syscall apis (to associate 'handles' with
> > > > internal structures), don't WARN with invalid input.
> > > > 
> > > > For example, POSIX timers are identified by timer_t id. These
> > > > ids are idr values. If userspace passes a representable timer_t id
> > > > value (eg, id < 0) but which was not previous allocated (since the
> > > > current idr api does not return negative idr values), then the
> > > > syscall properly returns an error; a WARN is unnecessary and
> > > > undesirable.
> > > 
> > > WARN_ON() on negative was intentional.  Because the previous
> > > implmentation silently dropped the msb, we at least wanna know who has
> > > been passing in negative indices.  Patch for the lock_timer is already
> > > in -mm.
> > 
> > Thanks. Can you share the patch title? It's not in next-20130225.
> 
> It's this one.
> 
>  http://thread.gmane.org/gmane.linux.kernel/1444883/focus=1445094

Thanks and sorry; not sure how I missed that thread.