lkml.org 
[lkml]   [2020]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()'
    On Wed, Mar 25 2020, Christophe JAILLET wrote:

    > Le 25/03/2020 à 15:52, Chuck Lever a écrit :
    >> Hi Christophe,
    >>
    >>
    >>> On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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@wanadoo.fr>
    >>> ---
    >>> 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);
    >>>
    >>> --

    Can I suggest something more like this:

    diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
    index de3c077733a7..0292f45b70f6 100644
    --- a/net/sunrpc/svc_xprt.c
    +++ b/net/sunrpc/svc_xprt.c
    @@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen)
    buf[0] = '\0';

    spin_lock(&svc_xprt_class_lock);
    - 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)
    - break;
    - len += slen;
    - strcat(buf, tmpstr);
    - }
    + list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list)
    + len += scnprintf(buf + len, maxlen - len, "%s %d\n",
    + xcl->xcl_name, xcl->xcl_max_payload);
    spin_unlock(&svc_xprt_class_lock);

    return len;
    NeilBrown
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2020-03-25 23:53    [W:3.610 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site