Re: [PATCH v1 1/1] SUNRPC: Make enough room in servername[] for AF_UNIX addresses

[Mirsad Todorovac <mtodorovac69@gmail.com>]

Hi, Neil,

Apparently I was duplicating work.

However, using

	char servername[UNIX_PATH_MAX];

has some advantages when compared to hard-coded integer?

Correct me if I'm wrong.

Best regards,
Mirsad Todorovac

On 9/23/24 23:24, NeilBrown wrote:
> On Tue, 24 Sep 2024, Mirsad Todorovac wrote:
>> GCC 13.2.0 reported with W=1 build option the following warning:
> 
> See
>   https://lore.kernel.org/all/20240814093853.48657-1-kunwu.chan@linux.dev/
> 
> I don't think anyone really cares about this one.
> 
> NeilBrown
> 
> 
>>
>> net/sunrpc/clnt.c: In function ‘rpc_create’:
>> net/sunrpc/clnt.c:582:75: warning: ‘%s’ directive output may be truncated writing up to 107 bytes into \
>> 					a region of size 48 [-Wformat-truncation=]
>>   582 |                                 snprintf(servername, sizeof(servername), "%s",
>>       |                                                                           ^~
>> net/sunrpc/clnt.c:582:33: note: ‘snprintf’ output between 1 and 108 bytes into a destination of size 48
>>   582 |                                 snprintf(servername, sizeof(servername), "%s",
>>       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   583 |                                          sun->sun_path);
>>       |                                          ~~~~~~~~~~~~~~
>>
>>    548         };
>>  → 549         char servername[48];
>>    550         struct rpc_clnt *clnt;
>>    551         int i;
>>    552
>>    553         if (args->bc_xprt) {
>>    554                 WARN_ON_ONCE(!(args->protocol & XPRT_TRANSPORT_BC));
>>    555                 xprt = args->bc_xprt->xpt_bc_xprt;
>>    556                 if (xprt) {
>>    557                         xprt_get(xprt);
>>    558                         return rpc_create_xprt(args, xprt);
>>    559                 }
>>    560         }
>>    561
>>    562         if (args->flags & RPC_CLNT_CREATE_INFINITE_SLOTS)
>>    563                 xprtargs.flags |= XPRT_CREATE_INFINITE_SLOTS;
>>    564         if (args->flags & RPC_CLNT_CREATE_NO_IDLE_TIMEOUT)
>>    565                 xprtargs.flags |= XPRT_CREATE_NO_IDLE_TIMEOUT;
>>    566         /*
>>    567          * If the caller chooses not to specify a hostname, whip
>>    568          * up a string representation of the passed-in address.
>>    569          */
>>    570         if (xprtargs.servername == NULL) {
>>    571                 struct sockaddr_un *sun =
>>    572                                 (struct sockaddr_un *)args->address;
>>    573                 struct sockaddr_in *sin =
>>    574                                 (struct sockaddr_in *)args->address;
>>    575                 struct sockaddr_in6 *sin6 =
>>    576                                 (struct sockaddr_in6 *)args->address;
>>    577
>>    578                 servername[0] = '\0';
>>    579                 switch (args->address->sa_family) {
>>  → 580                 case AF_LOCAL:
>>  → 581                         if (sun->sun_path[0])
>>  → 582                                 snprintf(servername, sizeof(servername), "%s",
>>  → 583                                          sun->sun_path);
>>  → 584                         else
>>  → 585                                 snprintf(servername, sizeof(servername), "@%s",
>>  → 586                                          sun->sun_path+1);
>>  → 587                         break;
>>    588                 case AF_INET:
>>    589                         snprintf(servername, sizeof(servername), "%pI4",
>>    590                                  &sin->sin_addr.s_addr);
>>    591                         break;
>>    592                 case AF_INET6:
>>    593                         snprintf(servername, sizeof(servername), "%pI6",
>>    594                                  &sin6->sin6_addr);
>>    595                         break;
>>    596                 default:
>>    597                         /* caller wants default server name, but
>>    598                          * address family isn't recognized. */
>>    599                         return ERR_PTR(-EINVAL);
>>    600                 }
>>    601                 xprtargs.servername = servername;
>>    602         }
>>    603
>>    604         xprt = xprt_create_transport(&xprtargs);
>>    605         if (IS_ERR(xprt))
>>    606                 return (struct rpc_clnt *)xprt;
>>
>> After the address family AF_LOCAL was added in the commit 176e21ee2ec89, the old hard-coded
>> size for servername of char servername[48] no longer fits. The maximum AF_UNIX address size
>> has now grown to UNIX_PATH_MAX defined as 108 in "include/uapi/linux/un.h" .
>>
>> The lines 580-587 were added later, addressing the leading zero byte '\0', but did not fix
>> the hard-coded servername limit.
>>
>> The AF_UNIX address was truncated to 47 bytes + terminating null byte. This patch will fix the
>> servername in AF_UNIX family to the maximum permitted by the system:
>>
>>    548         };
>>  → 549         char servername[UNIX_PATH_MAX];
>>    550         struct rpc_clnt *clnt;
>>
>> Fixes: 4388ce05fa38b ("SUNRPC: support abstract unix socket addresses")
>> Fixes: 510deb0d7035d ("SUNRPC: rpc_create() default hostname should support AF_INET6 addresses")
>> Fixes: 176e21ee2ec89 ("SUNRPC: Support for RPC over AF_LOCAL transports")
>> Cc: Neil Brown <neilb@suse.de>
>> Cc: Chuck Lever <chuck.lever@oracle.com>
>> Cc: Trond Myklebust <trondmy@kernel.org>
>> Cc: Anna Schumaker <anna@kernel.org>
>> Cc: Jeff Layton <jlayton@kernel.org>
>> Cc: Olga Kornievskaia <okorniev@redhat.com>
>> Cc: Dai Ngo <Dai.Ngo@oracle.com>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: linux-nfs@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
>> ---
>>  v1:
>> 	initial version.
>>
>>  net/sunrpc/clnt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 09f29a95f2bc..67099719893e 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
>>  		.connect_timeout = args->connect_timeout,
>>  		.reconnect_timeout = args->reconnect_timeout,
>>  	};
>> -	char servername[48];
>> +	char servername[UNIX_PATH_MAX];
>>  	struct rpc_clnt *clnt;
>>  	int i;
>>  
>> -- 
>> 2.43.0
>>
>>
>