Re: A potential race
Hi! We have no hardware to test possible fixes. If somebody has it and agrees to check our patches, we will prepare them. Best regards, Pavel 01.07.2016 20:17, Hans Verkuil пишет: On 07/01/2016 05:02 PM, Pavel Andrianov wrote: 01.07.2016 19:53, Hans Verkuil пишет: On 07/01/2016 04:39 PM, Pavel Andrianov wrote: Hi! There is a potential race condition between usbvision_v4l2_close and usbvision_disconnect. The possible scenario may be the following. usbvision_disconnect starts execution, assigns usbvision->remove_pending = 1, and is interrupted (rescheduled) after mutex_unlock. After that usbvision_v4l2_close is executed, decrease usbvision->user-- , checks usbvision->remove_pending, executes usbvision_release and finishes. Then usbvision_disconnect continues its execution. It checks usbversion->user (it is already 0) and also execute usbvision_release. Thus, release is executed twice. The same situation may occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. Moreover, the same problem is in usbvision_radio_close. In all these cases the check before call usbvision_release under mutex_lock protection does not solve the problem, because there may occur an open() after the check and the race takes place again. The question is: why the usbvision_release is called from close() (usbvision_v4l2_close and usbvision_radio_close)? Usually release functions are called from disconnect. Please don't use html mail, mailinglists will silently reject this. The usbvision driver is old and unloved and known to be very bad code. It needs a huge amount of work to make all this work correctly. I don't see anyone picking this up... Regards, Hans If you know the driver, could you, please, explain me, why usbvision_release is called from close functions (usbvision_v4l2_close and usbvision_radio_close) and not only from disconnect? Thanks! Because the author didn't know what he was doing. Although, to be fair, we have much better solutions for this. But who is willing to put in the time to fix this properly? The basic idea was: if someone still has a video/radio node open when disconnect happens, then we leave it to the last close to call release, otherwise we can call release right away. It needs to be rewritten. If you're volunteering to clean this up, then I can give pointers. Regards, Hans -- Pavel Andrianov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: andria...@ispras.ru -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A potential race
On 07/01/2016 05:02 PM, Pavel Andrianov wrote: > 01.07.2016 19:53, Hans Verkuil пишет: >> On 07/01/2016 04:39 PM, Pavel Andrianov wrote: >>> Hi! >>> >>> There is a potential race condition between usbvision_v4l2_close and >>> usbvision_disconnect. The possible scenario may be the following. >>> usbvision_disconnect starts execution, assigns usbvision->remove_pending = >>> 1, and is interrupted >>> (rescheduled) after mutex_unlock. After that usbvision_v4l2_close is >>> executed, decrease usbvision->user-- , checks usbvision->remove_pending, >>> executes usbvision_release and finishes. Then usbvision_disconnect >>> continues its execution. It checks >>> usbversion->user (it is already 0) and also execute usbvision_release. >>> Thus, release is executed twice. The same situation may >>> occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. >>> Moreover, the same problem is in usbvision_radio_close. In all these cases >>> the check before call usbvision_release under mutex_lock protection does >>> not solve the problem, because there may occur an open() after the check >>> and the race takes place again. The question is: why the usbvision_release >>> is called from close() (usbvision_v4l2_close and usbvision_radio_close)? >>> Usually release functions are called from >>> disconnect. >> Please don't use html mail, mailinglists will silently reject this. >> >> The usbvision driver is old and unloved and known to be very bad code. It >> needs a huge amount of work to make all this work correctly. >> >> I don't see anyone picking this up... >> >> Regards, >> >> Hans > If you know the driver, could you, please, explain me, why > usbvision_release is called from close functions (usbvision_v4l2_close > and usbvision_radio_close) and not only from disconnect? Thanks! > Because the author didn't know what he was doing. Although, to be fair, we have much better solutions for this. But who is willing to put in the time to fix this properly? The basic idea was: if someone still has a video/radio node open when disconnect happens, then we leave it to the last close to call release, otherwise we can call release right away. It needs to be rewritten. If you're volunteering to clean this up, then I can give pointers. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A potential race
01.07.2016 19:53, Hans Verkuil пишет: On 07/01/2016 04:39 PM, Pavel Andrianov wrote: Hi! There is a potential race condition between usbvision_v4l2_close and usbvision_disconnect. The possible scenario may be the following. usbvision_disconnect starts execution, assigns usbvision->remove_pending = 1, and is interrupted (rescheduled) after mutex_unlock. After that usbvision_v4l2_close is executed, decrease usbvision->user-- , checks usbvision->remove_pending, executes usbvision_release and finishes. Then usbvision_disconnect continues its execution. It checks usbversion->user (it is already 0) and also execute usbvision_release. Thus, release is executed twice. The same situation may occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. Moreover, the same problem is in usbvision_radio_close. In all these cases the check before call usbvision_release under mutex_lock protection does not solve the problem, because there may occur an open() after the check and the race takes place again. The question is: why the usbvision_release is called from close() (usbvision_v4l2_close and usbvision_radio_close)? Usually release functions are called from disconnect. Please don't use html mail, mailinglists will silently reject this. The usbvision driver is old and unloved and known to be very bad code. It needs a huge amount of work to make all this work correctly. I don't see anyone picking this up... Regards, Hans If you know the driver, could you, please, explain me, why usbvision_release is called from close functions (usbvision_v4l2_close and usbvision_radio_close) and not only from disconnect? Thanks! -- Pavel Andrianov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: andria...@ispras.ru -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A potential race
On 07/01/2016 04:39 PM, Pavel Andrianov wrote: > Hi! > > There is a potential race condition between usbvision_v4l2_close > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L403> > and usbvision_disconnect > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1569>. > The possible scenario may be the following. > usbvision_disconnect > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1569> > starts execution, assigns > usbvision->remove_pending = 1 > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1587>, > and is interrupted > (rescheduled) after mutex_unlock > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1592>. > After that > usbvision_v4l2_close > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L403> > is executed, decrease > usbvision->user-- > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L419>, > checks > usbvision->remove_pending > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L422>, > executes > usbvision_release <http://lxr.free-electrons.com/ident?i=usbvision_release> > and finishes. Then usbvision_disconnect > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1569> > continues its execution. It checks > usbversion->user > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1594> > (it is already 0) and also > execute usbvision_release > <http://lxr.free-electrons.com/ident?i=usbvision_release>. Thus, release is > executed twice. The same situation may > occur if usbvision_v4l2_close > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L403> > is interrupted by > usbvision_disconnect > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1569>. > Moreover, the same problem > is in usbvision_radio_close > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1135>. > In all these cases > the check before call usbvision_release > <http://lxr.free-electrons.com/ident?i=usbvision_release> under mutex_lock > protection does not solve > the problem, because there may occur an open() after the check and the race > takes place again. The question is: why the usbvision_release > <http://lxr.free-electrons.com/ident?i=usbvision_release> is called from > close() (usbvision_v4l2_close > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L403> > and usbvision_radio_close > <http://lxr.free-electrons.com/source/drivers/media/usb/usbvision/usbvision-video.c#L1135>)? > Usually release functions are called from > disconnect. Please don't use html mail, mailinglists will silently reject this. The usbvision driver is old and unloved and known to be very bad code. It needs a huge amount of work to make all this work correctly. I don't see anyone picking this up... Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
