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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/25/24 23:42, NeilBrown wrote:
> On Thu, 26 Sep 2024, Mirsad Todorovac wrote:
>> On 9/24/24 12:43, NeilBrown wrote:
>>> On Tue, 24 Sep 2024, Mirsad Todorovac wrote:
>>>> 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.
>>>
>>> I think you are wrong.  I agree that 48 is a poor choice.  I think that
>>> UNIX_PATH_MAX is a poor choice too.  The "servername" string is used for
>>> things other than a UNIX socket path.
>>> Did you read all of the thread that I provided a link for?  I suggest a
>>> more meaningful number in one of the later messages.
>>
>> I see. Thanks for the tip. However, if UNIX_PATH_MAX ever changes in the
>> future, the decl
>>
>>     char servername[108];
>>
>> might be missed when fixing all the changes caused by the change of the
>> macro definition? Am I wrong again?
> 
> Realistically UNIX_PATH_MAX is never going to change, and if it did that
> would not affect the correctness of this code.
> 
>>
>> Making it logically depend on the system limits might save some headache
>> in the future, perhaps.
> 
> Unlikely.  Did you look to see what the failure mode is here?
> servername is only ever used in log messages.  Truncating names in log
> message at 8 bytes can certainly be annoying.  Truncating at 48 bytes is
> much less of a problem.

Well, I agree that in this particular case you are correct, but this is not
about you or me being right or wrong, but that a macro would be more descriptive
than "48" or "108", unless you are used to count the beads while chanting mantras
:-)

>> If really the biggest string that will be copied there is: "/var/run/rpcbind.sock",
>> you are then right - stack space is precious commodity, and allocating
>> via kmalloc() might preempt the caller thread.
> 
> It might.  But the string is always passed to xprt_create_transport()
> which always calls kstrdup() on it.  So maybe that doesn't matter.  (As
> I said, understanding the big picture is important).

Ah. I agree, this is why I consider myself a disciple. While in the old
system it was considered asa breach of teacher's authority, other schools
of thinking encouraged questions and debate. ;-)

>> However, you got to this five weeks earlier - but the patch did not
>> propagate to the main vanilla torvalds tree.
> 
> Actually it has.
> 
> Commit 9090a7f78623 ("SUNRPC: Fix -Wformat-truncation warning")
> 
> $ git show --format=fuller  9090a7f78623 | grep CommitDate
> CommitDate: Mon Sep 23 15:03:13 2024 -0400
> 
> Linus merged it 
> Commit 684a64bf32b6 ("Merge tag 'nfs-for-6.12-1' of git://git.linux-nfs.org/projects/anna/linux-nfs")
> Date:   Tue Sep 24 15:44:18 2024 -0700
> 
> That patch used RPC_MAXNETNAMELEN which is the least-ugly simple fix.

Yes, my build was somewhat before that date:

-rw-r--r-- 1 root root 18977280 Sep 21 15:44 vmlinuz-6.11.0-gc13-x86-64-tmem-07462-g1868f9d0260e

But I whole-heartedly agree with not taking a literal integer. As we build things to last,
in a "design for life", so I believe that self-documenting source isan advantage to the
counting of places where some integers appear, and what each one meant.

As I said before, this is nothing target SUNRPC implementation, it was among the first compiler
warnings with make W=1 in build. I apologise if that sounded personal.

> Thanks for your interest in improving Linux.

You are most welcome. Thank you for your kind remark.

I am glad that you recognise my bug reports as a positive contribution, despite sometimes (or often)
duplicating work. But, please understand, on my level I do not even have enough disk space for all
the trees, let alone building all of them.

Maybe the best I could promise is to test linux-next and net-next trees.

Probably I could give more meaningful contribution with some expert advice, even when this episode
on SUNRPC was not a waste of time, but an opportunity to learn from the best. :-)

Best regards,
Mirsad Todorovac

> NeilBrown
> 
>>
>> Best regards,
>> Mirsad Todorovac
>>
>>> But I really think that the problem here is the warning.  The servername
>>> array is ALWAYS big enough for any string that will actually be copied
>>> into it.  gcc isn't clever enough to detect that, but it tries to be
>>> clever enough to tell you that even though you used snprintf so you know
>>> that the string might in theory overflow, it decides to warn you about
>>> something you already know.
>>>
>>> i.e.  if you want to fix this, I would rather you complain the the
>>> compiler writers.  Or maybe suggest a #pragma to silence the warning in
>>> this case.  Or maybe examine all of the code instead of the one line
>>> that triggers the warning and see if there is a better approach to
>>> providing the functionality that is being provided here.
>>>
>>> NeilBrown
>>>
>>>>
>>>> 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@xxxxxxxxx/
>>>>>
>>>>> 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@xxxxxxx>
>>>>>> Cc: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>>>>> Cc: Trond Myklebust <trondmy@xxxxxxxxxx>
>>>>>> Cc: Anna Schumaker <anna@xxxxxxxxxx>
>>>>>> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
>>>>>> Cc: Olga Kornievskaia <okorniev@xxxxxxxxxx>
>>>>>> Cc: Dai Ngo <Dai.Ngo@xxxxxxxxxx>
>>>>>> Cc: Tom Talpey <tom@xxxxxxxxxx>
>>>>>> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
>>>>>> Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
>>>>>> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
>>>>>> Cc: Paolo Abeni <pabeni@xxxxxxxxxx>
>>>>>> Cc: linux-nfs@xxxxxxxxxxxxxxx
>>>>>> Cc: netdev@xxxxxxxxxxxxxxx
>>>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>>>> Signed-off-by: Mirsad Todorovac <mtodorovac69@xxxxxxxxx>
>>>>>> ---
>>>>>>  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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux