Received-SPF: pass (google.com: domain of linux-nfs+bounces-165-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <da202cf6-b836-4b9d-9da7-dc9613e936d0@redhat.com>
Date: Wed, 29 Nov 2023 07:34:33 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH nfs-utils v2 1/2] fsidd: call anonymous sockets by their
 name only, don't fill with NULs to 108 bytes
Content-Language: en-US
To: =?UTF-8?Q?Ahelenia_Ziemia=C5=84ska?= <nabijaczleweli@nabijaczleweli.xyz>,
 linux-nfs@vger.kernel.org, NeilBrown <neilb@suse.de>
References: <b38ecca96762d939d377c381bf34521ee5945129.1700601199.git.nabijaczleweli@nabijaczleweli.xyz>
From: Steve Dickson <steved@redhat.com>
In-Reply-To: <b38ecca96762d939d377c381bf34521ee5945129.1700601199.git.nabijaczleweli@nabijaczleweli.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 11/21/23 4:14 PM, Ahelenia Ziemiańska wrote:
> Since e00ab3c0616fe6d83ab0710d9e7d989c299088f7, ss -l looks like this:
>    u_seq               LISTEN                0                     5                                    @/run/fsid.sock@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ 26989379                                                       * 0
> with fsidd pushing all the addresses to 108 bytes wide, which is deeply
> egregious if you don't filter it out and recolumnate.
> 
> This is because, naturally (unix(7)), "Null bytes in the name have
> no special significance": abstract addresses are binary blobs, but
> paths automatically terminate at the first NUL byte, since paths
> can't contain those.
> 
> So just specify the correct address length when we're using the abstract domain:
> unix(7) recommends "offsetof(struct sockaddr_un, sun_path) + strlen(sun_path) + 1"
> for paths, but we don't want to include the terminating NUL, so it's just
> "offsetof(struct sockaddr_un, sun_path) + strlen(sun_path)".
> This brings the width back to order:
> -- >8 --
> $ ss -la | grep @
> u_str ESTAB     0      0      @45208536ec96909a/bus/systemd-timesyn/bus-api-timesync 18500238                            * 18501249
> u_str ESTAB     0      0       @fecc9657d2315eb7/bus/systemd-network/bus-api-network 18495452                            * 18494406
> u_seq LISTEN    0      5                                             @/run/fsid.sock 27168796                            * 0
> u_str ESTAB     0      0                 @ac308f35f50797a2/bus/systemd-logind/system 19406                               * 15153
> u_str ESTAB     0      0                @b6606e0dfacbae75/bus/systemd/bus-api-system 18494353                            * 18495334
> u_str ESTAB     0      0                    @5880653d215718a7/bus/systemd/bus-system 26930876                            * 26930003
> -- >8 --
> 
> Fixes: e00ab3c0616fe6d83ab0710d9e7d989c299088f7 ("fsidd: provide
>   better default socket name.")
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Committed... (tag: nfs-utils-2-7-1-rc1)

steved
> ---
> v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz>
> v2 NFC, addr_len declared at top of function
> 
>   support/reexport/fsidd.c    | 9 ++++++---
>   support/reexport/reexport.c | 8 ++++++--
>   2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c
> index 3e62b3fc..8a70b78f 100644
> --- a/support/reexport/fsidd.c
> +++ b/support/reexport/fsidd.c
> @@ -147,6 +147,7 @@ int main(void)
>   {
>   	struct event *srv_ev;
>   	struct sockaddr_un addr;
> +	socklen_t addr_len;
>   	char *sock_file;
>   	int srv;
>   
> @@ -161,10 +162,12 @@ int main(void)
>   	memset(&addr, 0, sizeof(struct sockaddr_un));
>   	addr.sun_family = AF_UNIX;
>   	strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
> -	if (addr.sun_path[0] == '@')
> +	addr_len = sizeof(struct sockaddr_un);
> +	if (addr.sun_path[0] == '@') {
>   		/* "abstract" socket namespace */
> +		addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path);
>   		addr.sun_path[0] = 0;
> -	else
> +	} else
>   		unlink(sock_file);
>   
>   	srv = socket(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK, 0);
> @@ -173,7 +176,7 @@ int main(void)
>   		return 1;
>   	}
>   
> -	if (bind(srv, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un)) == -1) {
> +	if (bind(srv, (const struct sockaddr *)&addr, addr_len) == -1) {
>   		xlog(L_WARNING, "Unable to bind %s: %m\n", sock_file);
>   		return 1;
>   	}
> diff --git a/support/reexport/reexport.c b/support/reexport/reexport.c
> index 78516586..0fb49a46 100644
> --- a/support/reexport/reexport.c
> +++ b/support/reexport/reexport.c
> @@ -21,6 +21,7 @@ static int fsidd_srv = -1;
>   static bool connect_fsid_service(void)
>   {
>   	struct sockaddr_un addr;
> +	socklen_t addr_len;
>   	char *sock_file;
>   	int ret;
>   	int s;
> @@ -33,9 +34,12 @@ static bool connect_fsid_service(void)
>   	memset(&addr, 0, sizeof(struct sockaddr_un));
>   	addr.sun_family = AF_UNIX;
>   	strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
> -	if (addr.sun_path[0] == '@')
> +	addr_len = sizeof(struct sockaddr_un);
> +	if (addr.sun_path[0] == '@') {
>   		/* "abstract" socket namespace */
> +		addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path);
>   		addr.sun_path[0] = 0;
> +	}
>   
>   	s = socket(AF_UNIX, SOCK_SEQPACKET, 0);
>   	if (s == -1) {
> @@ -43,7 +47,7 @@ static bool connect_fsid_service(void)
>   		return false;
>   	}
>   
> -	ret = connect(s, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un));
> +	ret = connect(s, (const struct sockaddr *)&addr, addr_len);
>   	if (ret == -1) {
>   		xlog(L_WARNING, "Unable to connect %s: %m, is fsidd running?\n", sock_file);
>   		return false;