[libcamera-devel] libcamera: v4l2_device: openat(2) with O_CLOEXEC to cleanup after exec(3)

18380 diff mbox series

It's generally a good idea to openat(2) with O_CLOEXEC, but this patch
also fixes a problem where the v4l2 devices would report -EBUSY after
an exec(3).

Signed-off-by: Elias Naur <mail@eliasnaur.com>
---
 src/libcamera/v4l2_device.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Elias,

Thank you for the patch.

On Sat, Mar 11, 2023 at 01:05:25PM -0600, Elias Naur via libcamera-devel wrote:
> It's generally a good idea to openat(2) with O_CLOEXEC, but this patch
> also fixes a problem where the v4l2 devices would report -EBUSY after
> an exec(3).

Could you elaborate a bit on this -EBUSY problem ?

> Signed-off-by: Elias Naur <mail@eliasnaur.com>
> ---
>  src/libcamera/v4l2_device.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp
> index 57a88d96..ad9d1e37 100644
> --- src/libcamera/v4l2_device.cpp
> +++ src/libcamera/v4l2_device.cpp
> @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags)
>  		return -EBUSY;
>  	}
>  
> -	UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));
> +	UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC));

There should be spaces around the '|'. The utils/checkstyle.py script
should have reported that. You can automate running checkstyle.py as a
git post-commit (or pre-commit, if desired) hook by copying
utils/hooks/post-commit to .git/hooks/.

There's no need to submit a v2 of this patch just for this, I can fix it
locally.

>  	if (!fd.isValid()) {
>  		int ret = -errno;
>  		LOG(V4L2, Error) << "Failed to open V4L2 device '"

Hi Elias, Laurent

On Sun, Mar 12, 2023 at 11:39:30AM +0200, Laurent Pinchart via libcamera-devel wrote:
> Hi Elias,
>
> Thank you for the patch.
>
> On Sat, Mar 11, 2023 at 01:05:25PM -0600, Elias Naur via libcamera-devel wrote:
> > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch
> > also fixes a problem where the v4l2 devices would report -EBUSY after
> > an exec(3).
>
> Could you elaborate a bit on this -EBUSY problem ?
>

I also wonder if you got into a real bug or this is just potentially
dangerous. In example I know "libcamerify" execs, did you get issues
with the file descriptor being kept open across it ?


> > Signed-off-by: Elias Naur <mail@eliasnaur.com>
> > ---
> >  src/libcamera/v4l2_device.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp
> > index 57a88d96..ad9d1e37 100644
> > --- src/libcamera/v4l2_device.cpp
> > +++ src/libcamera/v4l2_device.cpp
> > @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags)
> >  		return -EBUSY;
> >  	}
> >
> > -	UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));
> > +	UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC));
>
> There should be spaces around the '|'. The utils/checkstyle.py script
> should have reported that. You can automate running checkstyle.py as a
> git post-commit (or pre-commit, if desired) hook by copying
> utils/hooks/post-commit to .git/hooks/.
>
> There's no need to submit a v2 of this patch just for this, I can fix it
> locally.
>
Anyway, I think this is generally a good idea and with Laurent's
comment on style addressed at apply time

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> >  	if (!fd.isValid()) {
> >  		int ret = -errno;
> >  		LOG(V4L2, Error) << "Failed to open V4L2 device '"
>
> --
> Regards,
>
> Laurent Pinchart

Thank you for your reviews. See updated patch[0] that correct the
style issue and elaborates
the problem O_CLOEXEC solves for me.

[0] https://lists.libcamera.org/pipermail/libcamera-devel/2023-March/037100.html

On Mon, 13 Mar 2023 at 02:29, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Elias, Laurent
>
> On Sun, Mar 12, 2023 at 11:39:30AM +0200, Laurent Pinchart via libcamera-devel wrote:
> > Hi Elias,
> >
> > Thank you for the patch.
> >
> > On Sat, Mar 11, 2023 at 01:05:25PM -0600, Elias Naur via libcamera-devel wrote:
> > > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch
> > > also fixes a problem where the v4l2 devices would report -EBUSY after
> > > an exec(3).
> >
> > Could you elaborate a bit on this -EBUSY problem ?
> >
>
> I also wonder if you got into a real bug or this is just potentially
> dangerous. In example I know "libcamerify" execs, did you get issues
> with the file descriptor being kept open across it ?
>

It's a weird corner-case: a self-updating executable for an appliance-style
deployment of a buildroot-like environment. I hope I explained the problem
better in the v2 patch.

>
> > > Signed-off-by: Elias Naur <mail@eliasnaur.com>
> > > ---
> > >  src/libcamera/v4l2_device.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp
> > > index 57a88d96..ad9d1e37 100644
> > > --- src/libcamera/v4l2_device.cpp
> > > +++ src/libcamera/v4l2_device.cpp
> > > @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags)
> > >             return -EBUSY;
> > >     }
> > >
> > > -   UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));
> > > +   UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC));
> >
> > There should be spaces around the '|'. The utils/checkstyle.py script
> > should have reported that. You can automate running checkstyle.py as a
> > git post-commit (or pre-commit, if desired) hook by copying
> > utils/hooks/post-commit to .git/hooks/.
> >
> > There's no need to submit a v2 of this patch just for this, I can fix it
> > locally.
> >
> Anyway, I think this is generally a good idea and with Laurent's
> comment on style addressed at apply time
>

Addressed in v2.