Received-SPF: pass (google.com: domain of linux-nfs+bounces-2176-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 4 Mar 2024 19:42:29 +0100
From: Petr Vorel <pvorel@suse.cz>
To: NeilBrown <neilb@suse.de>
Cc: Steve Dickson <steved@redhat.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 3/4] Listen on an AF_UNIX abstract address if supported.
Message-ID: <20240304184229.GC3408054@pevik>
Reply-To: Petr Vorel <pvorel@suse.cz>
References: <20240225235628.12473-1-neilb@suse.de>
 <20240225235628.12473-4-neilb@suse.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240225235628.12473-4-neilb@suse.de>

Hi Neil, Steve,

> As RPC is primarily a network service it is best, on Linux, to use
> network namespaces to isolate it.  However contacting rpcbind via an
> AF_UNIX socket allows escape from the network namespace.
> If clients could use an abstract address, that would ensure clients
> contact an rpcbind in the same network namespace.

> systemd can pass in a listening abstract socket by providing an '@'
> prefix.  However with libtirpc 1.3.3 or earlier attempting this will
> fail as the library mistakenly determines that the socket is not bound.
> This generates unsightly error messages.
> So it is best not to request the abstract address when it is not likely
> to work.

> A patch to fix this also proposes adding a define for
> _PATH_RPCBINDSOCK_ABSTRACT to the header files.  We can check for this
> and only include the new ListenStream when that define is present.

> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  configure.ac                                  | 13 ++++++++++++-
>  systemd/{rpcbind.socket => rpcbind.socket.in} |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
>  rename systemd/{rpcbind.socket => rpcbind.socket.in} (88%)
NOTE: now systemd/rpcbind.socket should be in .gitignore.

The rest LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> diff --git a/configure.ac b/configure.ac
> index c2069a2b3b0e..573e4fdf3a3e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -50,6 +50,17 @@ AC_SUBST([nss_modules], [$with_nss_modules])

>  PKG_CHECK_MODULES([TIRPC], [libtirpc])

> +CPPFLAGS=$TIRPC_CFLAGS
> +AC_MSG_CHECKING([for abstract socket support in libtirpc])
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
> +#include <rpc/rpc.h>
> +],[
> +char *path = _PATH_RPCBINDSOCK_ABSTRACT;
> +])], [have_abstract=yes], [have_abstract=no])
> +CPPFLAGS=
> +AC_MSG_RESULT([$have_abstract])
> +AM_CONDITIONAL(ABSTRACT, [ test "x$have_abstract" = "xyes" ])
> +
>  PKG_PROG_PKG_CONFIG
>  AC_ARG_WITH([systemdsystemunitdir],
>    AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files]),
> @@ -76,4 +87,4 @@ AC_CHECK_HEADERS([nss.h])
>  AC_SUBST([_sbindir])
>  AC_CONFIG_COMMANDS_PRE([eval eval _sbindir=$sbindir])

> -AC_OUTPUT([Makefile systemd/rpcbind.service])
> +AC_OUTPUT([Makefile systemd/rpcbind.service systemd/rpcbind.socket])
> diff --git a/systemd/rpcbind.socket b/systemd/rpcbind.socket.in
> similarity index 88%
> rename from systemd/rpcbind.socket
> rename to systemd/rpcbind.socket.in
> index 3b1a93694c21..5dd09a143e16 100644
> --- a/systemd/rpcbind.socket
> +++ b/systemd/rpcbind.socket.in
> @@ -6,6 +6,7 @@ Before=rpcbind.target

>  [Socket]
>  ListenStream=/run/rpcbind.sock
> +@ABSTRACT_TRUE@ListenStream=@/run/rpcbind.sock

>  # RPC netconfig can't handle ipv6/ipv4 dual sockets
>  BindIPv6Only=ipv6-only