Re: [PATCH] posix-timer: don't call idr_find() w/ negative ID

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 20 Feb 2013, Tejun Heo wrote:

> Recent idr updates make idr_find() trigger WARN_ON_ONCE() before
> returning NULL when a negative ID is specified.  Apparently,
> posix-timer::__lock_timer() was depending on idr_find() returning NULL
> on negative ID, thus triggering the new WARN_ON_ONCE().  Make
> __lock_timer() first check whether @timer_id is negative and return
> NULL without invoking idr_find() if so.

I can grumpily accept the patch below as a quick hack fix, which can
go to stable as well, but not with such a patently misleading
changelog.

The changelog wants to document, that this is not a proper fix at all
and just a quick hack which can be nonintrusively applied to stable.

> Note that the previous code was theoretically broken.  idr_find()
> masked off the sign bit before performing lookup and if the matching
> IDs were in use, it would have returned pointer for the incorrect
> entry.

Brilliant code that. What's the purpose of having the idr id as an
"int" and then masking off the sign bit instead of simply refusing
negative id values in the idr code itself or simply making the id
"unsigned int" ?

Just look at the various f*ckedup users of MAX_IDR_MASK. Example:

drivers/pps/pps.c:

        err = idr_get_new(&pps_idr, pps, &pps->id);
        mutex_unlock(&pps_idr_lock);

        if (err < 0)
                return err;

        pps->id &= MAX_IDR_MASK;

Why the heck is that necessary? Either we get an error code and we
don't care about pps->id or idr_get_new() sets pps->id to a valid idr
id. If the idr code really returns a _valid_ id with the sign bit set,
then the idr code is broken beyond repair and needs to be fixed
instead of propagating that insanity all over the users.

Thanks,

	tglx