On Thu, 28 Feb 2013 10:08:46 +0100
[email protected] wrote:
> Just hit the following warning on current git tree:
>
> ------------[ cut here ]------------
> WARNING: at lib/idr.c:678 idr_find_slowpath+0x97/0xc0()
> Hardware name: System Product Name
> Pid: 12366, comm: okular Not tainted 3.8.0-09633-g2a7d2b9-dirty #312
> Call Trace:
> [<ffffffff8105b7a0>] ? warn_slowpath_common+0x60/0xa0
> [<ffffffff811bf557>] ? idr_find_slowpath+0x97/0xc0
> [<ffffffff811249f2>] ? inotify_idr_find_locked+0x32/0x80
> [<ffffffff810ecf7d>] ? fput+0x1d/0xc0
> [<ffffffff811254d0>] ? sys_inotify_rm_watch+0x50/0xc0
> [<ffffffff814c9f58>] ? int_signal+0x12/0x17
> [<ffffffff814c9d52>] ? system_call_fastpath+0x16/0x1b
okluar passed a negative `wd' into inotify_rm_watch()?
I wonder why it did that. I doubt if inotify_add_watch() ever returns
negative descriptors, in which case I expect that okular's call to
inotify_add_watch() returned -1 and an errno and okular forgot to check
it, and later passed that -1 back into inotify_rm_watch().
Anyway, I guess we need to make inotify_add_watch() refuse to return
negative descriptors (presumably this is already the case due to idr
internals) and make inotify_rm_watch() trap negative values of `wd' up
front.
I wonder how many other such gremlins the IDR checking has added.
Hello, Andrew.
On Fri, Mar 01, 2013 at 01:31:59PM -0800, Andrew Morton wrote:
> okluar passed a negative `wd' into inotify_rm_watch()?
>
> I wonder why it did that. I doubt if inotify_add_watch() ever returns
> negative descriptors, in which case I expect that okular's call to
> inotify_add_watch() returned -1 and an errno and okular forgot to check
> it, and later passed that -1 back into inotify_rm_watch().
>
> Anyway, I guess we need to make inotify_add_watch() refuse to return
> negative descriptors (presumably this is already the case due to idr
> internals) and make inotify_rm_watch() trap negative values of `wd' up
> front.
>
> I wonder how many other such gremlins the IDR checking has added.
The WARN_ON() is just for cases where someone might be doing something
crazy with the previous behavior of ignoring high bit. Maybe I was
being overly paranoid and we should just drop it from idr_find().
Thanks.
--
tejun
idr_find(), idr_remove() and idr_replace() used to silently ignore the
sign bit and perform lookup with the rest of the bits. The weird
behavior has been changed such that negative IDs are treated as
invalid. As the behavior change was subtle, WARN_ON_ONCE() was added
in the hope of determining who's calling idr functions with negative
IDs so that they can be examined for problems.
Up until now, all two reported cases are ID number coming directly
from userland and getting fed into idr_find() and the warnings seem to
cause more problems than being helpful. Drop the WARN_ON_ONCE()s.
Signed-off-by: Tejun Heo <[email protected]>
Reported-by: [email protected]
Cc: Andrew Morton <[email protected]>
---
lib/idr.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/lib/idr.c b/lib/idr.c
index 73f4d53..00739aa 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -569,8 +569,7 @@ void idr_remove(struct idr *idp, int id)
struct idr_layer *p;
struct idr_layer *to_free;
- /* see comment in idr_find_slowpath() */
- if (WARN_ON_ONCE(id < 0))
+ if (id < 0)
return;
sub_remove(idp, (idp->layers - 1) * IDR_BITS, id);
@@ -667,15 +666,7 @@ void *idr_find_slowpath(struct idr *idp, int id)
int n;
struct idr_layer *p;
- /*
- * If @id is negative, idr_find() used to ignore the sign bit and
- * performed lookup with the rest of bits, which is weird and can
- * lead to very obscure bugs. We're now returning NULL for all
- * negative IDs but just in case somebody was depending on the sign
- * bit being ignored, let's trigger WARN_ON_ONCE() so that they can
- * be detected and fixed. WARN_ON_ONCE() can later be removed.
- */
- if (WARN_ON_ONCE(id < 0))
+ if (id < 0)
return NULL;
p = rcu_dereference_raw(idp->top);
@@ -824,8 +815,7 @@ void *idr_replace(struct idr *idp, void *ptr, int id)
int n;
struct idr_layer *p, *old_p;
- /* see comment in idr_find_slowpath() */
- if (WARN_ON_ONCE(id < 0))
+ if (id < 0)
return ERR_PTR(-EINVAL);
p = idp->top;
On Fri, 2013-03-01 at 13:35 -0800, Tejun Heo wrote:
> Hello, Andrew.
>
> On Fri, Mar 01, 2013 at 01:31:59PM -0800, Andrew Morton wrote:
> > okluar passed a negative `wd' into inotify_rm_watch()?
> >
> > I wonder why it did that. I doubt if inotify_add_watch() ever returns
> > negative descriptors, in which case I expect that okular's call to
> > inotify_add_watch() returned -1 and an errno and okular forgot to check
> > it, and later passed that -1 back into inotify_rm_watch().
> >
> > Anyway, I guess we need to make inotify_add_watch() refuse to return
> > negative descriptors (presumably this is already the case due to idr
> > internals) and make inotify_rm_watch() trap negative values of `wd' up
> > front.
> >
> > I wonder how many other such gremlins the IDR checking has added.
>
> The WARN_ON() is just for cases where someone might be doing something
> crazy with the previous behavior of ignoring high bit. Maybe I was
> being overly paranoid and we should just drop it from idr_find().
Can we revert the __lock_timer patch as well then?
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
>
On Fri, Mar 1, 2013 at 1:54 PM, Peter Hurley <[email protected]> wrote:
>> The WARN_ON() is just for cases where someone might be doing something
>> crazy with the previous behavior of ignoring high bit. Maybe I was
>> being overly paranoid and we should just drop it from idr_find().
>
> Can we revert the __lock_timer patch as well then?
I don't know. Andrew was worried about the type of timer id. For
inotify, it's okay as the type is always int but it's not a bad idea
to have some form of sanitizing if the type might deviate.
Thanks.
--
tejun
On Fri, 2013-03-01 at 13:56 -0800, Tejun Heo wrote:
> On Fri, Mar 1, 2013 at 1:54 PM, Peter Hurley <[email protected]> wrote:
> >> The WARN_ON() is just for cases where someone might be doing something
> >> crazy with the previous behavior of ignoring high bit. Maybe I was
> >> being overly paranoid and we should just drop it from idr_find().
> >
> > Can we revert the __lock_timer patch as well then?
>
> I don't know. Andrew was worried about the type of timer id. For
> inotify, it's okay as the type is always int but it's not a bad idea
> to have some form of sanitizing if the type might deviate.
It's the other way around that would be the problem, if idr returned a
type that wasn't representable by timer_t.
IMO, idr should be the sanitizing; either the value is valid and found
or not. But that's just my opinion :)
Regards,
Peter Hurley