Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

GIT version control wrote:
> Module: xenomai-jki
> Branch: for-upstream
> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
> 
> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> Date:   Tue Mar 30 11:13:33 2010 +0200
> 
> RTDM: Fix potential NULL pointer dereference
> 
> The rework in 95278926edc559d4 misses the case that context can be NULL,
> which can (and has) triggered a kernel oops. Take care of this case.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

I still think that fix is a useles waste of time. Let us merge
Philippe's patches instead.


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> GIT version control wrote:
>> Module: xenomai-jki
>> Branch: for-upstream
>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>
>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>
>> RTDM: Fix potential NULL pointer dereference
>>
>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>> which can (and has) triggered a kernel oops. Take care of this case.
>>
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
> I still think that fix is a useles waste of time. Let us merge
> Philippe's patches instead.

Please accept that Philippe's patch is orthogonal to this bug.

And it didn't work as-is. I'll post a rework which has the same benefit
(avoiding to poll on pending context references) - once it is tested.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> GIT version control wrote:
>>> Module: xenomai-jki
>>> Branch: for-upstream
>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>
>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>>
>>> RTDM: Fix potential NULL pointer dereference
>>>
>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>>> which can (and has) triggered a kernel oops. Take care of this case.
>>>
>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> I still think that fix is a useles waste of time. Let us merge
>> Philippe's patches instead.
> 
> Please accept that Philippe's patch is orthogonal to this bug.
> 
> And it didn't work as-is. I'll post a rework which has the same benefit
> (avoiding to poll on pending context references) - once it is tested.

Ok. I am fine with any variation as long as:
- close returns immediately even if the request is not taken into
account immediately;
- the file descriptor index is available again as soon as close returns;
- the kernel objects attached to the file descriptor are destroyed when
the last reference to it is closed.

In shoft: POSIX conformance.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> GIT version control wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-upstream
>>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>
>>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>>>
>>>> RTDM: Fix potential NULL pointer dereference
>>>>
>>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>>>> which can (and has) triggered a kernel oops. Take care of this case.
>>>>
>>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> I still think that fix is a useles waste of time. Let us merge
>>> Philippe's patches instead.
>> Please accept that Philippe's patch is orthogonal to this bug.
>>
>> And it didn't work as-is. I'll post a rework which has the same benefit
>> (avoiding to poll on pending context references) - once it is tested.
> 
> Ok. I am fine with any variation as long as:
> - close returns immediately even if the request is not taken into
> account immediately;
> - the file descriptor index is available again as soon as close returns;
> - the kernel objects attached to the file descriptor are destroyed when
> the last reference to it is closed.

That's precisely what I implemented. Additionally, I had to take care of
RTDM drivers deferring the close via EAGAIN and some other minor aspects.

> 
> In shoft: POSIX conformance.

At least blocking has nothing to do with POSIX (some drivers will
continue to block in their close handlers). And - AFAIU - the order of
releasing the fd internally and blocking on something during close is
not specified.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> GIT version control wrote:
>>>>> Module: xenomai-jki
>>>>> Branch: for-upstream
>>>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>>
>>>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>>>>
>>>>> RTDM: Fix potential NULL pointer dereference
>>>>>
>>>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>>>>> which can (and has) triggered a kernel oops. Take care of this case.
>>>>>
>>>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>> I still think that fix is a useles waste of time. Let us merge
>>>> Philippe's patches instead.
>>> Please accept that Philippe's patch is orthogonal to this bug.
>>>
>>> And it didn't work as-is. I'll post a rework which has the same benefit
>>> (avoiding to poll on pending context references) - once it is tested.
>> Ok. I am fine with any variation as long as:
>> - close returns immediately even if the request is not taken into
>> account immediately;
>> - the file descriptor index is available again as soon as close returns;
>> - the kernel objects attached to the file descriptor are destroyed when
>> the last reference to it is closed.
> 
> That's precisely what I implemented. Additionally, I had to take care of
> RTDM drivers deferring the close via EAGAIN and some other minor aspects.

I am afraid EAGAIN gets translated automatically into ENOMERGE ;-)

> 
>> In shoft: POSIX conformance.
> 
> At least blocking has nothing to do with POSIX (some drivers will
> continue to block in their close handlers). And - AFAIU - the order of
> releasing the fd internally and blocking on something during close is
> not specified.

The point is that the close handler should not be called when close is
called, but when the last reference to the file descriptor is closed,
asynchronously if need be. So, it may block. But the close service
should return immediately.

Maybe it is not POSIX, but it is the way it should be, and the way
people expect a sane driver API to answer. Crappy drivers which do not
answer to SIGINT are simply not acceptable and only a waste developer
time (and POSIX mandates EINTR in that case).

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> GIT version control wrote:
>>>>>> Module: xenomai-jki
>>>>>> Branch: for-upstream
>>>>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>>>
>>>>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>>>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>>>>>
>>>>>> RTDM: Fix potential NULL pointer dereference
>>>>>>
>>>>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>>>>>> which can (and has) triggered a kernel oops. Take care of this case.
>>>>>>
>>>>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> I still think that fix is a useles waste of time. Let us merge
>>>>> Philippe's patches instead.
>>>> Please accept that Philippe's patch is orthogonal to this bug.
>>>>
>>>> And it didn't work as-is. I'll post a rework which has the same benefit
>>>> (avoiding to poll on pending context references) - once it is tested.
>>> Ok. I am fine with any variation as long as:
>>> - close returns immediately even if the request is not taken into
>>> account immediately;
>>> - the file descriptor index is available again as soon as close returns;
>>> - the kernel objects attached to the file descriptor are destroyed when
>>> the last reference to it is closed.
>> That's precisely what I implemented. Additionally, I had to take care of
>> RTDM drivers deferring the close via EAGAIN and some other minor aspects.
> 
> I am afraid EAGAIN gets translated automatically into ENOMERGE ;-)

Sorry, I'm also concerned about legacy compatibility. So this is a
must-have.

But don't worry, this is an internal detail between driver and RTDM.
Neither the user nor drivers that makes no use of it will notice.

> 
>>> In shoft: POSIX conformance.
>> At least blocking has nothing to do with POSIX (some drivers will
>> continue to block in their close handlers). And - AFAIU - the order of
>> releasing the fd internally and blocking on something during close is
>> not specified.
> 
> The point is that the close handler should not be called when close is
> called, but when the last reference to the file descriptor is closed,
> asynchronously if need be. So, it may block. But the close service
> should return immediately.
> 
> Maybe it is not POSIX, but it is the way it should be, and the way
> people expect a sane driver API to answer. Crappy drivers which do not
> answer to SIGINT are simply not acceptable and only a waste developer
> time (and POSIX mandates EINTR in that case).

We can't change these bits. The close handler will continue to be called
when the request is issued and possibly once again (or more if it asks
for this via EAGAIN) when the final release happens.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> GIT version control wrote:
>>>>>>> Module: xenomai-jki
>>>>>>> Branch: for-upstream
>>>>>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>>>>
>>>>>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>>>>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>>>>>>
>>>>>>> RTDM: Fix potential NULL pointer dereference
>>>>>>>
>>>>>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>>>>>>> which can (and has) triggered a kernel oops. Take care of this case.
>>>>>>>
>>>>>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>> I still think that fix is a useles waste of time. Let us merge
>>>>>> Philippe's patches instead.
>>>>> Please accept that Philippe's patch is orthogonal to this bug.
>>>>>
>>>>> And it didn't work as-is. I'll post a rework which has the same benefit
>>>>> (avoiding to poll on pending context references) - once it is tested.
>>>> Ok. I am fine with any variation as long as:
>>>> - close returns immediately even if the request is not taken into
>>>> account immediately;
>>>> - the file descriptor index is available again as soon as close returns;
>>>> - the kernel objects attached to the file descriptor are destroyed when
>>>> the last reference to it is closed.
>>> That's precisely what I implemented. Additionally, I had to take care of
>>> RTDM drivers deferring the close via EAGAIN and some other minor aspects.
>> I am afraid EAGAIN gets translated automatically into ENOMERGE ;-)
> 
> Sorry, I'm also concerned about legacy compatibility. So this is a
> must-have.
> 
> But don't worry, this is an internal detail between driver and RTDM.
> Neither the user nor drivers that makes no use of it will notice.
> 
>>>> In shoft: POSIX conformance.
>>> At least blocking has nothing to do with POSIX (some drivers will
>>> continue to block in their close handlers). And - AFAIU - the order of
>>> releasing the fd internally and blocking on something during close is
>>> not specified.
>> The point is that the close handler should not be called when close is
>> called, but when the last reference to the file descriptor is closed,
>> asynchronously if need be. So, it may block. But the close service
>> should return immediately.
>>
>> Maybe it is not POSIX, but it is the way it should be, and the way
>> people expect a sane driver API to answer. Crappy drivers which do not
>> answer to SIGINT are simply not acceptable and only a waste developer
>> time (and POSIX mandates EINTR in that case).
> 
> We can't change these bits. The close handler will continue to be called
> when the request is issued and possibly once again (or more if it asks
> for this via EAGAIN) when the final release happens.

As fare as I remember Philippe's patch fixed these bits. I have no
problem if the polling happens in a kernel thread (or workqueue, as far
as I remember Philippe's patch), and the close service has already
returned and freed the file descriptor.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Philippe Gerum]

On Fri, 2010-04-16 at 14:31 +0200, Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
> > GIT version control wrote:
> >> Module: xenomai-jki
> >> Branch: for-upstream
> >> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
> >> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
> >>
> >> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> >> Date:   Tue Mar 30 11:13:33 2010 +0200
> >>
> >> RTDM: Fix potential NULL pointer dereference
> >>
> >> The rework in 95278926edc559d4 misses the case that context can be NULL,
> >> which can (and has) triggered a kernel oops. Take care of this case.
> >>
> >> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> > 
> > I still think that fix is a useles waste of time. Let us merge
> > Philippe's patches instead.
> 
> Please accept that Philippe's patch is orthogonal to this bug.
> 
> And it didn't work as-is. I'll post a rework which has the same benefit
> (avoiding to poll on pending context references) - once it is tested.

You mean that your current implementation, does poll, right? Because
mine does not, that was the point of it, also to solve races in
cross-context management IIRC. That patch dates back to the 2.5 -rc
cycle, when I needed it to have a decent behavior while working on the
RTIPC stuff. It should not be that difficult to forward port it to
-head, I guess. Let me know if I can help.

> 
> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Jan Kiszka]

Philippe Gerum wrote:
> On Fri, 2010-04-16 at 14:31 +0200, Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> GIT version control wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-upstream
>>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
>>>>
>>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>> Date:   Tue Mar 30 11:13:33 2010 +0200
>>>>
>>>> RTDM: Fix potential NULL pointer dereference
>>>>
>>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
>>>> which can (and has) triggered a kernel oops. Take care of this case.
>>>>
>>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> I still think that fix is a useles waste of time. Let us merge
>>> Philippe's patches instead.
>> Please accept that Philippe's patch is orthogonal to this bug.
>>
>> And it didn't work as-is. I'll post a rework which has the same benefit
>> (avoiding to poll on pending context references) - once it is tested.
> 
> You mean that your current implementation, does poll, right? Because
> mine does not, that was the point of it, also to solve races in
> cross-context management IIRC.

Polling is part of the RTDM API: If a driver requests it via EAGAIN, we
have to provide it. We may deprecate this feature long-term, but for now
there are no drop-in replacements available and we have drivers in the
field that make use of it.

> That patch dates back to the 2.5 -rc
> cycle, when I needed it to have a decent behavior while working on the
> RTIPC stuff. It should not be that difficult to forward port it to
> -head, I guess. Let me know if I can help.

Thanks. The patch is done, it just waits for the final tests as I wrote.
I would be happy if you could test it as well if you have something that
nicely stresses e.g. RTIPC in this domain.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Wolfgang Mauerer : RTDM: Fix potential NULL pointer dereference

[Philippe Gerum]

On Fri, 2010-04-16 at 16:04 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Fri, 2010-04-16 at 14:31 +0200, Jan Kiszka wrote:
> >> Gilles Chanteperdrix wrote:
> >>> GIT version control wrote:
> >>>> Module: xenomai-jki
> >>>> Branch: for-upstream
> >>>> Commit: 55ebde80258b5b6c3d29d37b5f30a3199faf0881
> >>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=55ebde80258b5b6c3d29d37b5f30a3199faf0881
> >>>>
> >>>> Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> >>>> Date:   Tue Mar 30 11:13:33 2010 +0200
> >>>>
> >>>> RTDM: Fix potential NULL pointer dereference
> >>>>
> >>>> The rework in 95278926edc559d4 misses the case that context can be NULL,
> >>>> which can (and has) triggered a kernel oops. Take care of this case.
> >>>>
> >>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> >>> I still think that fix is a useles waste of time. Let us merge
> >>> Philippe's patches instead.
> >> Please accept that Philippe's patch is orthogonal to this bug.
> >>
> >> And it didn't work as-is. I'll post a rework which has the same benefit
> >> (avoiding to poll on pending context references) - once it is tested.
> > 
> > You mean that your current implementation, does poll, right? Because
> > mine does not, that was the point of it, also to solve races in
> > cross-context management IIRC.
> 
> Polling is part of the RTDM API: If a driver requests it via EAGAIN, we
> have to provide it. We may deprecate this feature long-term, but for now
> there are no drop-in replacements available and we have drivers in the
> field that make use of it.

Well, you likely know my feeling about this. Being bug-compatible
induces more issues than it solves in the long run. As a frequent RTDM
user myself, I must admit that the close issue has been a real PITA over
time, and I guess a lot more people are suffering in silence. So
anything to solve it properly, I mean following the POLA, would be
warmly welcomed. POSIX is sometimes silly, but follows POLA for sure.

> 
> > That patch dates back to the 2.5 -rc
> > cycle, when I needed it to have a decent behavior while working on the
> > RTIPC stuff. It should not be that difficult to forward port it to
> > -head, I guess. Let me know if I can help.
> 
> Thanks. The patch is done, it just waits for the final tests as I wrote.
> I would be happy if you could test it as well if you have something that
> nicely stresses e.g. RTIPC in this domain.
> 

Ok, I can do that. Let me know when I can try things.

> Jan
> 


-- 
Philippe.