[libcamera-devel] libcamera: process: Fail loudly on isolate
diff mbox series

Message ID 20190719080556.26785-1-jacopo@jmondi.org
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: process: Fail loudly on isolate
Related show

Commit Message

Jacopo Mondi July 19, 2019, 8:05 a.m. UTC 
Add an error debug message when disassociating part of a process
execution context using unshare fails.

As this is currently used to isolate a child process which is
immediately terminated silently if unshare fails, add a debug printout
and propagate up the error code to make the failure more visible.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/process.cpp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--
2.21.0

Comments

Kieran Bingham July 19, 2019, 11:51 a.m. UTC | #1 
Hi Jacopo,

On 19/07/2019 09:05, Jacopo Mondi wrote:
> Add an error debug message when disassociating part of a process
> execution context using unshare fails.
> 
> As this is currently used to isolate a child process which is
> immediately terminated silently if unshare fails, add a debug printout
> and propagate up the error code to make the failure more visible.
> 

This sounds good to me.

Have you added this because it has happened to you ?

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/process.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 6c41da219f05..ab716a9cd57f 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -306,7 +306,15 @@ void Process::closeAllFdsExcept(const std::vector<int> &fds)
> 
>  int Process::isolate()
>  {
> -	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Process, Error) << "Failed to unshare execution context: "
> +				    << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  /**
> --
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Niklas Söderlund July 20, 2019, 1:07 p.m. UTC | #2 
Hi Jacopo,

Thanks for your work.

On 2019-07-19 10:05:56 +0200, Jacopo Mondi wrote:
> Add an error debug message when disassociating part of a process
> execution context using unshare fails.
> 
> As this is currently used to isolate a child process which is
> immediately terminated silently if unshare fails, add a debug printout
> and propagate up the error code to make the failure more visible.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/process.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 6c41da219f05..ab716a9cd57f 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -306,7 +306,15 @@ void Process::closeAllFdsExcept(const std::vector<int> &fds)
> 
>  int Process::isolate()
>  {
> -	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Process, Error) << "Failed to unshare execution context: "
> +				    << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  /**
> --
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Paul Elder July 25, 2019, 8:38 a.m. UTC | #3 
On Fri, Jul 19, 2019 at 10:05:56AM +0200, Jacopo Mondi wrote:
> Add an error debug message when disassociating part of a process
> execution context using unshare fails.
> 
> As this is currently used to isolate a child process which is
> immediately terminated silently if unshare fails, add a debug printout
> and propagate up the error code to make the failure more visible.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/process.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 6c41da219f05..ab716a9cd57f 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -306,7 +306,15 @@ void Process::closeAllFdsExcept(const std::vector<int> &fds)
> 
>  int Process::isolate()
>  {
> -	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Process, Error) << "Failed to unshare execution context: "
> +				    << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  /**
> --
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 30, 2019, 4:13 a.m. UTC | #4 
Hi Jacopo,

Thank you for the patch.

On Fri, Jul 19, 2019 at 10:05:56AM +0200, Jacopo Mondi wrote:
> Add an error debug message when disassociating part of a process
> execution context using unshare fails.
> 
> As this is currently used to isolate a child process which is
> immediately terminated silently if unshare fails, add a debug printout
> and propagate up the error code to make the failure more visible.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/process.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 6c41da219f05..ab716a9cd57f 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -306,7 +306,15 @@ void Process::closeAllFdsExcept(const std::vector<int> &fds)
> 
>  int Process::isolate()
>  {
> -	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Process, Error) << "Failed to unshare execution context: "
> +				    << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
>  }

I wonder if we shouldn't add the error message in the caller instead, as
the isolate() method is expected to grow, but for now this should be
good enough and can be changed later if needed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Please push :-)

> 
>  /**
Jacopo Mondi July 31, 2019, 1:29 p.m. UTC | #5 
Hi Kieran,

On Fri, Jul 19, 2019 at 12:51:54PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 19/07/2019 09:05, Jacopo Mondi wrote:
> > Add an error debug message when disassociating part of a process
> > execution context using unshare fails.
> >
> > As this is currently used to isolate a child process which is
> > immediately terminated silently if unshare fails, add a debug printout
> > and propagate up the error code to make the failure more visible.
> >
>
> This sounds good to me.
>
> Have you added this because it has happened to you ?

Yes, isolate() fails on my device with -EPERM...
The only explanation I have is that CLONE_NEWUSER and CLONE_NEWNET
both requires CAP_SYS_ADMIN capability. I've just superficially looked
into this, but the printout helped me understand what was happening.

Thanks
  j

>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/process.cpp | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> > index 6c41da219f05..ab716a9cd57f 100644
> > --- a/src/libcamera/process.cpp
> > +++ b/src/libcamera/process.cpp
> > @@ -306,7 +306,15 @@ void Process::closeAllFdsExcept(const std::vector<int> &fds)
> >
> >  int Process::isolate()
> >  {
> > -	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
> > +	int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
> > +	if (ret) {
> > +		ret = -errno;
> > +		LOG(Process, Error) << "Failed to unshare execution context: "
> > +				    << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> >  }
> >
> >  /**
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran

Patch
diff mbox series 

diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
index 6c41da219f05..ab716a9cd57f 100644
--- a/src/libcamera/process.cpp
+++ b/src/libcamera/process.cpp
@@ -306,7 +306,15 @@  void Process::closeAllFdsExcept(const std::vector<int> &fds)

 int Process::isolate()
 {
-	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
+	int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
+	if (ret) {
+		ret = -errno;
+		LOG(Process, Error) << "Failed to unshare execution context: "
+				    << strerror(-ret);
+		return ret;
+	}
+
+	return 0;
 }

 /**