Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210402094042.9218-1-gi-oh.kim@ionos.com> <CAKwvOdkFJ_WFpt2+rnNR3tbrdFky2NnEOWSG7MhgLrBHJAOEVw@mail.gmail.com>
In-Reply-To: <CAKwvOdkFJ_WFpt2+rnNR3tbrdFky2NnEOWSG7MhgLrBHJAOEVw@mail.gmail.com>
From:   Gioh Kim <gi-oh.kim@ionos.com>
Date:   Fri, 2 Apr 2021 21:41:38 +0200
Message-ID: <CAJX1YtbvVCkzP9AyTd_hZXzN9LV=YgxOZRO17zZRCgTB=MFj3w@mail.gmail.com>
Subject: Re: [PATCH] lib/string: Introduce sysfs_streqcase
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Jinpu Wang <jinpu.wang@ionos.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 2, 2021 at 8:17 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Thanks for the patch!
>
> + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
>
> On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner. I found some cases using strncasecmp to check the entire
> > strings and they would not work as intended.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> > And also drivers/pnp/interface.c checks "disable" command with
> > strncasecmp but it would also work if the command is "disable-wrong".
>
> Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
>
> Also, if they're being liberal in accepting either case, I don't see
> why the sysfs nodes should be strict in rejecting trailing input at
> that point.
>


On Fri, Apr 2, 2021 at 8:17 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Thanks for the patch!
>
> + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
>
> On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner. I found some cases using strncasecmp to check the entire
> > strings and they would not work as intended.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> > And also drivers/pnp/interface.c checks "disable" command with
> > strncasecmp but it would also work if the command is "disable-wrong".
>
> Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
>
> Also, if they're being liberal in accepting either case, I don't see
> why the sysfs nodes should be strict in rejecting trailing input at
> that point.
>

strcasecmp does not work when a user inputs the command with echo.
We can force the human to use 'echo -n' but there are also some applications
that pass the command with \n. If the command includes \n, strcasecmp does
not work.

In short, I need a function working well for both case-insensitive string and
a string followed by '\n'.

I am not native speaker of English. I think the below example can show
my problem.

Below is the original code. That code does not work because of the \n
in the command.

char buf[] = "mi\n";

if (strcasecmp(buf, "min-inflight")  == 0 ||
    strcasecmp(buf, "mi") == 0)
    printf("inflight\n");
else if (strcasecmp(buf, "min-latency") == 0 ||
    strcasecmp(buf, "ml") == 0)
    printf("latency\n");
else
    printf("wrong\n");

Below is the current code in RTRS module. We replaced strcasecmp with
strncasecmp.
That works well but ugly and error-prone.

size_t len = 0;
len = strlen(buf);
if (buf[len - 1] == '\n')
    len--;
if (strncasecmp(buf, "min-inflight", 12)  == 0 ||
    (len == 2 && strncasecmp(buf, "mi", 2) == 0))
    printf("inflight\n");
else if (strncasecmp(buf, "min-latency", 11) == 0 ||
    (len == 2 && strncasecmp(buf, "ml", 2)) == 0)
    printf("latency\n");
else
    printf("wrong\n");

I think sysfs_streqcase could be the best option as below.

if (sysfs_streqcase(buf, "min-inflight") ||
    sysfs_streqcase(buf, "mi"))
    printf("inflight\n");
else if (sysfs_streqcase(buf, "min-latency") ||
    sysfs_streqcase(buf, "ml"))
    printf("latency\n");
else
    printf("wrong\n");


I think that case is not my own problem.
I think some code handling debugfs and sysfs also have the same problem.

>
> This should be declared in
> include/linux/string.h
> in order for others to use this (as 0day bot notes).

Thank you for the kind review.
I will add the declaration if I get the positive feedback for sysfs_streqcase.

>
> > +
> >  /**
> >   * match_string - matches given string in an array
> >   * @array:     array of strings
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers