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".
Signed-off-by: Gioh Kim <[email protected]>
---
lib/string.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/lib/string.c b/lib/string.c
index 7548eb715ddb..5e6bc0d3d5c6 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
}
EXPORT_SYMBOL(sysfs_streq);
+/**
+ * sysfs_streqcase - same to sysfs_streq and case insensitive
+ * @s1: one string
+ * @s2: another string
+ *
+ */
+bool sysfs_streqcase(const char *s1, const char *s2)
+{
+ while (*s1 && tolower(*s1) == tolower(*s2)) {
+ s1++;
+ s2++;
+ }
+
+ if (*s1 == *s2)
+ return true;
+ if (!*s1 && *s2 == '\n' && !s2[1])
+ return true;
+ if (*s1 == '\n' && !s1[1] && !*s2)
+ return true;
+ return false;
+}
+EXPORT_SYMBOL(sysfs_streqcase);
+
/**
* match_string - matches given string in an array
* @array: array of strings
--
2.25.1
Hi Gioh,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gioh-Kim/lib-string-Introduce-sysfs_streqcase/20210402-174251
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5e46d1b78a03d52306f21f77a4e4a144b6d31486
config: mips-randconfig-r006-20210402 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project b23a314146956dd29b719ab537608ced736fc036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/0day-ci/linux/commit/b18d0dc7264473a4023926c510a67adfd7eaf119
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gioh-Kim/lib-string-Introduce-sysfs_streqcase/20210402-174251
git checkout b18d0dc7264473a4023926c510a67adfd7eaf119
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> lib/string.c:723:6: warning: no previous prototype for function 'sysfs_streqcase' [-Wmissing-prototypes]
bool sysfs_streqcase(const char *s1, const char *s2)
^
lib/string.c:723:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool sysfs_streqcase(const char *s1, const char *s2)
^
static
1 warning generated.
vim +/sysfs_streqcase +723 lib/string.c
716
717 /**
718 * sysfs_streqcase - same to sysfs_streq and case insensitive
719 * @s1: one string
720 * @s2: another string
721 *
722 */
> 723 bool sysfs_streqcase(const char *s1, const char *s2)
724 {
725 while (*s1 && tolower(*s1) == tolower(*s2)) {
726 s1++;
727 s2++;
728 }
729
730 if (*s1 == *s2)
731 return true;
732 if (!*s1 && *s2 == '\n' && !s2[1])
733 return true;
734 if (*s1 == '\n' && !s1[1] && !*s2)
735 return true;
736 return false;
737 }
738 EXPORT_SYMBOL(sysfs_streqcase);
739
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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 <[email protected]> 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.
>
> Signed-off-by: Gioh Kim <[email protected]>
> ---
> lib/string.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/lib/string.c b/lib/string.c
> index 7548eb715ddb..5e6bc0d3d5c6 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
> }
> EXPORT_SYMBOL(sysfs_streq);
>
> +/**
> + * sysfs_streqcase - same to sysfs_streq and case insensitive
> + * @s1: one string
> + * @s2: another string
> + *
> + */
> +bool sysfs_streqcase(const char *s1, const char *s2)
> +{
> + while (*s1 && tolower(*s1) == tolower(*s2)) {
> + s1++;
> + s2++;
> + }
> +
> + if (*s1 == *s2)
> + return true;
> + if (!*s1 && *s2 == '\n' && !s2[1])
> + return true;
> + if (*s1 == '\n' && !s1[1] && !*s2)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL(sysfs_streqcase);
This should be declared in
include/linux/string.h
in order for others to use this (as 0day bot notes).
> +
> /**
> * match_string - matches given string in an array
> * @array: array of strings
> --
> 2.25.1
>
--
Thanks,
~Nick Desaulniers
On Fri, Apr 02, 2021 at 11:17:13AM -0700, Nick Desaulniers 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 <[email protected]> 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.
I think this shouldn't be prefixed "sysfs_" -- name it for what it does,
not where it gets used, if it's a general utility function.
-Kees
>
> >
> > Signed-off-by: Gioh Kim <[email protected]>
> > ---
> > lib/string.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/lib/string.c b/lib/string.c
> > index 7548eb715ddb..5e6bc0d3d5c6 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
> > }
> > EXPORT_SYMBOL(sysfs_streq);
> >
> > +/**
> > + * sysfs_streqcase - same to sysfs_streq and case insensitive
> > + * @s1: one string
> > + * @s2: another string
> > + *
> > + */
> > +bool sysfs_streqcase(const char *s1, const char *s2)
> > +{
> > + while (*s1 && tolower(*s1) == tolower(*s2)) {
> > + s1++;
> > + s2++;
> > + }
> > +
> > + if (*s1 == *s2)
> > + return true;
> > + if (!*s1 && *s2 == '\n' && !s2[1])
> > + return true;
> > + if (*s1 == '\n' && !s1[1] && !*s2)
> > + return true;
> > + return false;
> > +}
> > +EXPORT_SYMBOL(sysfs_streqcase);
>
> This should be declared in
> include/linux/string.h
> in order for others to use this (as 0day bot notes).
>
> > +
> > /**
> > * match_string - matches given string in an array
> > * @array: array of strings
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Kees Cook
On Fri, Apr 2, 2021 at 8:24 PM Kees Cook <[email protected]> wrote:
>
> On Fri, Apr 02, 2021 at 11:17:13AM -0700, Nick Desaulniers 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 <[email protected]> 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.
>
> I think this shouldn't be prefixed "sysfs_" -- name it for what it does,
> not where it gets used, if it's a general utility function.
>
I think sysfs_streqcase is another version of sysfs_streq.
That is why I use "sysfs_" prefix.
On Fri, Apr 2, 2021 at 8:17 PM Nick Desaulniers <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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