Re: [patch 021/104] lib/idr.c: fix rcu related race with idr_find

[Dave Airlie]

On Wed, Dec 10, 2008 at 12:02 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 10 Dec 2008 11:46:13 +1000 "Dave Airlie" <airlied@xxxxxxxxx> wrote:
>
>> >>
>> >> On Wed, 10 Dec 2008, Dave Airlie wrote:
>> >>>
>> >>> On Thu, Dec 4, 2008 at 5:49 AM, Greg KH <gregkh@xxxxxxx> wrote:
>> >>> > 2.6.27-stable review patch.  If anyone has any objections, please let us know.
>> >>> >
>> >>> Revert.
>> >>>
>> >>> This caused problems in the F10 kernel with idr, the drm device alloc
>> >>> went all wierd,
>> >>> it might be a drm bug but changing this code triggers it and so it
>> >>> isn't really "stable"
>> >>
>> >> Well, maybe it should be reverted in mainlne too, then?
>> >
>> > It appears idr_replace is broken at least in stable with this patch.
>> >
>> > I'm trying to track down where the problem is (idr_replace doesn't look like
>> > idr_find in a lot of places and I wonder if this has ever been tested.)
>> >
>> (cc-trimmed).
>>
>> Okay I'm not idr expert and maybe what the drm is doing is illegal but
>> it never caused a problem up to now.
>>
>> The drm grabs an idr minor number using a NULL pointer to reserve the
>> number, it then uses idr_replace later
>> to stick a pointer into the reserved number. However this seems to be
>> what is broken, I'm not sure if this is a legal
>> use of idrs but has worked like that for a long time now.
>>
>> I can fix the drm to workaround this, and allocate my pointers before
>> I try to get a minor number, but I'd like to know
>> if my usage is illegal over just overlooked.
>
>
> <greps for a while>
>
> I assume we're talking about drivers/gpu/drm/drm_stub.c:drm_minor_get_id()?
>
> I don't immediately see anything in the idr code which special-cases a
> NULL caller pointer?
>

Actually now that I'm starting to wrap my head around it I think it
might be the fact that I call
idr_get_new_above with 64, then later with 0. I'm not sure the new
code is dealing with that case so
well.

We don't do that in the standard kernel tree yet, so it explains why
nobody's noticed, however the KMS
changes introduce it, and we have those in f10.

http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=blob;f=drivers/gpu/drm/drm_stub.c;h=5ca132afa4f2e128999e319e44e31ad156e6ab74;hb=drm-next

is the drm_stub.c from drm-next that will trigger the issue.

Again I'm not sure if this is a legal use of idrs.

Dave.

>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/