2021-04-08 13:09:30

by Gioh Kim

[permalink] [raw]
Subject: [PATCH v4] lib/string: Introduce sysfs_streqcase

As the name shows, it checks if strings are equal in case insensitive
manner.

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

I found some more cases using strncasecmp to check the entire string
such as rtrs-clt-sysfs.c does. 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]>
---
include/linux/string.h | 1 +
lib/string.c | 36 ++++++++++++++++++++++++++++--------
2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 4fcfb56abcf5..36d00ff8013e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -184,6 +184,7 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
extern void argv_free(char **argv);

extern bool sysfs_streq(const char *s1, const char *s2);
+extern bool sysfs_streqcase(const char *s1, const char *s2);
extern int kstrtobool(const char *s, bool *res);
static inline int strtobool(const char *s, bool *res)
{
diff --git a/lib/string.c b/lib/string.c
index 7548eb715ddb..d0fb02efd5da 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -687,6 +687,17 @@ char *strsep(char **s, const char *ct)
EXPORT_SYMBOL(strsep);
#endif

+static inline bool __streq_terminal(const char *s1, const char *s2)
+{
+ if (*s1 == *s2)
+ return true;
+ if (!*s1 && *s2 == '\n' && !s2[1])
+ return true;
+ if (*s1 == '\n' && !s1[1] && !*s2)
+ return true;
+ return false;
+}
+
/**
* sysfs_streq - return true if strings are equal, modulo trailing newline
* @s1: one string
@@ -703,17 +714,26 @@ bool sysfs_streq(const char *s1, const char *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;
+ return __streq_terminal(s1, 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++;
+ }
+ return __streq_terminal(s1, s2);
+}
+EXPORT_SYMBOL(sysfs_streqcase);
+
/**
* match_string - matches given string in an array
* @array: array of strings
--
2.25.1


2021-04-08 13:15:26

by Jinpu Wang

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Thu, Apr 8, 2021 at 3:06 PM Gioh Kim <[email protected]> wrote:
>
> As the name shows, it checks if strings are equal in case insensitive
> manner.
>
> 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".
>
> I found some more cases using strncasecmp to check the entire string
> such as rtrs-clt-sysfs.c does. 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]>
you should add the
Reported-by: kernel test robot <[email protected]>
> ---
you can add the changelog here after the ---
v4->v3: removed #ifdef CONFIG_SYSFS ~ #endif.

The string comparison doesn't depends on CONFIG_SYSFS at all.

It looks good to me.
Reviewed-by: Jack Wang <[email protected]>



> include/linux/string.h | 1 +
> lib/string.c | 36 ++++++++++++++++++++++++++++--------
> 2 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4fcfb56abcf5..36d00ff8013e 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -184,6 +184,7 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> extern void argv_free(char **argv);
>
> extern bool sysfs_streq(const char *s1, const char *s2);
> +extern bool sysfs_streqcase(const char *s1, const char *s2);
> extern int kstrtobool(const char *s, bool *res);
> static inline int strtobool(const char *s, bool *res)
> {
> diff --git a/lib/string.c b/lib/string.c
> index 7548eb715ddb..d0fb02efd5da 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -687,6 +687,17 @@ char *strsep(char **s, const char *ct)
> EXPORT_SYMBOL(strsep);
> #endif
>
> +static inline bool __streq_terminal(const char *s1, const char *s2)
> +{
> + if (*s1 == *s2)
> + return true;
> + if (!*s1 && *s2 == '\n' && !s2[1])
> + return true;
> + if (*s1 == '\n' && !s1[1] && !*s2)
> + return true;
> + return false;
> +}
> +
> /**
> * sysfs_streq - return true if strings are equal, modulo trailing newline
> * @s1: one string
> @@ -703,17 +714,26 @@ bool sysfs_streq(const char *s1, const char *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;
> + return __streq_terminal(s1, 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++;
> + }
> + return __streq_terminal(s1, s2);
> +}
> +EXPORT_SYMBOL(sysfs_streqcase);
> +
> /**
> * match_string - matches given string in an array
> * @array: array of strings
> --
> 2.25.1
>

2021-04-08 14:54:16

by Gioh Kim

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Thu, Apr 8, 2021 at 3:14 PM Jinpu Wang <[email protected]> wrote:
>
> On Thu, Apr 8, 2021 at 3:06 PM Gioh Kim <[email protected]> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner.
> >
> > 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".
> >
> > I found some more cases using strncasecmp to check the entire string
> > such as rtrs-clt-sysfs.c does. 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]>
> you should add the
> Reported-by: kernel test robot <[email protected]>
> > ---
> you can add the changelog here after the ---
> v4->v3: removed #ifdef CONFIG_SYSFS ~ #endif.
>
> The string comparison doesn't depends on CONFIG_SYSFS at all.
>
> It looks good to me.
> Reviewed-by: Jack Wang <[email protected]>
>
>

Yes, I got two build error reports for v3.
Should I send v5 including "Reported-by: kernel test robot <[email protected]>" tag?

2021-04-08 18:18:51

by Nick Desaulniers

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Thu, Apr 8, 2021 at 7:52 AM Gioh Kim <[email protected]> wrote:
>
> On Thu, Apr 8, 2021 at 3:14 PM Jinpu Wang <[email protected]> wrote:
> >
> > On Thu, Apr 8, 2021 at 3:06 PM Gioh Kim <[email protected]> wrote:
> > >
> > > As the name shows, it checks if strings are equal in case insensitive
> > > manner.
> > >
> > > 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".
> > >
> > > I found some more cases using strncasecmp to check the entire string
> > > such as rtrs-clt-sysfs.c does. 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]>

v4 LGTM, thanks.

Reviewed-by: Nick Desaulniers <[email protected]>

> > you should add the
> > Reported-by: kernel test robot <[email protected]>
> > > ---
> > you can add the changelog here after the ---
> > v4->v3: removed #ifdef CONFIG_SYSFS ~ #endif.
> >
> > The string comparison doesn't depends on CONFIG_SYSFS at all.
> >
> > It looks good to me.
> > Reviewed-by: Jack Wang <[email protected]>
> >
> >
>
> Yes, I got two build error reports for v3.
> Should I send v5 including "Reported-by: kernel test robot <[email protected]>" tag?

I don't think that's necessary. I would use that tag if I was fixing
an issue reported by the bot; but v4 is basically the same as v2 in
regards to the issue 0day bot reported with v3. v3 just demonstrates
that there are drivers with possibly incorrect Kconfig dependencies
(missing a dependency on SYSFS perhaps). So the underlying problem was
not reported by 0day bot; 0day bot just helped avoid issues from v3.

Fixing the Kconfig dependencies would be nice to have, but not a
requirement IMO to this feature/functionality.

--
Thanks,
~Nick Desaulniers

2021-04-09 05:07:03

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Thu, 8 Apr 2021 15:06:05 +0200 Gioh Kim <[email protected]> wrote:

> As the name shows, it checks if strings are equal in case insensitive
> manner.

Peh. Who would die if we simply made sysfs_streq() case-insensitive?

2021-04-09 06:46:25

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Thu, Apr 08, 2021 at 10:05:02PM -0700, Andrew Morton wrote:
> On Thu, 8 Apr 2021 15:06:05 +0200 Gioh Kim <[email protected]> wrote:
>
> > As the name shows, it checks if strings are equal in case insensitive
> > manner.
>
> Peh. Who would die if we simply made sysfs_streq() case-insensitive?

I doubt anyone, let's do that instead.

2021-04-09 06:52:50

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Fri, 9 Apr 2021 08:44:39 +0200 Greg Kroah-Hartman <[email protected]> wrote:

> On Thu, Apr 08, 2021 at 10:05:02PM -0700, Andrew Morton wrote:
> > On Thu, 8 Apr 2021 15:06:05 +0200 Gioh Kim <[email protected]> wrote:
> >
> > > As the name shows, it checks if strings are equal in case insensitive
> > > manner.
> >
> > Peh. Who would die if we simply made sysfs_streq() case-insensitive?
>
> I doubt anyone, let's do that instead.

There's a risk that people will write scripts/config/etc on a 5.13+
kernel and then find that they malfunction on earlier kernels...

2021-04-09 07:00:52

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Thu, Apr 08, 2021 at 11:51:32PM -0700, Andrew Morton wrote:
> On Fri, 9 Apr 2021 08:44:39 +0200 Greg Kroah-Hartman <[email protected]> wrote:
>
> > On Thu, Apr 08, 2021 at 10:05:02PM -0700, Andrew Morton wrote:
> > > On Thu, 8 Apr 2021 15:06:05 +0200 Gioh Kim <[email protected]> wrote:
> > >
> > > > As the name shows, it checks if strings are equal in case insensitive
> > > > manner.
> > >
> > > Peh. Who would die if we simply made sysfs_streq() case-insensitive?
> >
> > I doubt anyone, let's do that instead.
>
> There's a risk that people will write scripts/config/etc on a 5.13+
> kernel and then find that they malfunction on earlier kernels...
>

That's not a regression, that is a "newer kernels have newer features"
issue :)

2021-04-09 12:42:40

by Gioh Kim

[permalink] [raw]
Subject: Re: [PATCH v4] lib/string: Introduce sysfs_streqcase

On Fri, Apr 9, 2021 at 9:11 AM Andy Shevchenko
<[email protected]> wrote:
>
>
>
> On Friday, April 9, 2021, Andrew Morton <[email protected]> wrote:
>>
>> On Thu, 8 Apr 2021 15:06:05 +0200 Gioh Kim <[email protected]> wrote:
>>
>> > As the name shows, it checks if strings are equal in case insensitive
>> > manner.
>>
>> Peh. Who would die if we simply made sysfs_streq() case-insensitive?
>
>
> I think it will be a disaster. Like we have with case-insensitive file systems. Famous Mac case that most of the soft stop working when they actually moved to the right direction, I.e. case-sensitive. Personally I’m against such change.

Hi Andy,

I am sorry but I am a little bit confused because I don't have any
experience with case-insensitive file systems.
I think the case-insensitive file system does not care if sysfs_streq
is changed to case-insensitive.

For example, if the case-insensitive file system would accept the "OFF" command,
the user can pass either of "off" and "OFF" and both will work with
case-insensitive strfs_streq.

On the contrary, I think the case-sensitive file system would not like
it if sysfs_streq is made case-insensitive.

Could you please inform me what I am missing?

>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>