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 >>>>>> >>>>>> >>>>> >>>> >>> >> >