Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 20 Feb 2013 22:38:36 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH] posix-timer: don't call idr_find() w/ negative ID |
| |
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
| ![\](/images/icornerr.gif) |