Re: [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()'

[Christophe JAILLET]

Le 25/03/2020 Ã 15:52, Chuck Lever a ÃcritÂ:
> Hi Christophe,
>
>
> > On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
> >
> > Using 'snprintf' is safer than 'sprintf' because it can avoid a buffer
> > overflow.
> That's true as a general statement, but how likely is such an
> overflow to occur here?
I guess, that it us unlikely and that the 80 chars buffer is big enough.
That is the exact reason of why I've proposed 2 patches. The first one 
could happen in RL. The 2nd is more like a clean-up and is less 
relevant, IMHO.
> > The return value can also be used to avoid a strlen a call.
> That's also true of sprintf, isn't it?

Sure.


> > Finally, we know where we need to copy and the length to copy, so, we
> > can save a few cycles by rearraging the code and using a memcpy instead of
> > a strcat.
> I would be OK with squashing these two patches together. I don't
> see the need to keep the two changes separated.

NP, I can resend as a V2 with your comments.
As said above, the first fixes something that could, IMHO, happen and 
the 2nd is more a matter of taste and a clean-up.


> > Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > ---
> > This patch should have no functionnal change.
> > We could go further, use scnprintf and write directly in the destination
> > buffer. However, this could lead to a truncated last line.
> That's exactly what this function is trying to avoid. As part of any
> change in this area, it would be good to replace the current block
> comment before this function with a Doxygen-format comment that
> documents that goal.

I'll take care of it.


> > ---
> > net/sunrpc/svc_xprt.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index df39e7b8b06c..6df861650040 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -118,12 +118,12 @@ int svc_print_xprts(char *buf, int maxlen)
> > 	list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
> > 		int slen;
> >
> > -		sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload);
> > -		slen = strlen(tmpstr);
> > -		if (len + slen >= maxlen)
> > +		slen = snprintf(tmpstr, sizeof(tmpstr), "%s %d\n",
> > +				xcl->xcl_name, xcl->xcl_max_payload);
> > +		if (slen >= sizeof(tmpstr) || len + slen >= maxlen)
> > 			break;
> > +		memcpy(buf + len, tmpstr, slen + 1);
> > 		len += slen;
> > -		strcat(buf, tmpstr);
> IMO replacing the strcat makes the code harder to read, and this
> is certainly not a performance path. Can you drop that part of the
> patch?

Ok


> > 	}
> > 	spin_unlock(&svc_xprt_class_lock);
> >
> > --
> > 2.20.1
> --
> Chuck Lever