Re: Writing to a const pointer: is this supposed to happen?

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Sunday, July 05, 2020 21:05 CEST, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: 
> On Sunday, July 5, 2020, Kars Mulder <kerneldev@karsmulder.nl> wrote:
> > On Saturday, July 04, 2020 22:54 CEST, Andy Shevchenko wrote:
> > > This and similar are not correct. 1/ They are not replacement per se
> > > (because of different behaviour). 2/ They simple_strto*() are not
> > > obsoleted.
> > >
> > > Can you correct all places you found and make it consistent?
> >
> > Something like the following patch? It changes all occurrences of
> > "replacement" or "obsolete" (that I'm aware of) with "preferred over".
> >
> Yes, thanks!

Okay. Then I shall start writing a changelog and formally submit this patch.

... While I'm at that, I realised that I had taken the liberty to correct
the references to the functions they replace (e.g. kstrtol now says it's
preferred over simple_strtol rather than simple_strtoull), which is
a semantically different change than changing replacement/obsolete to
"preferred over".

Does this mean that I should should split my patch into two patches?

---
 include/linux/kernel.h |  4 ++--
 lib/kstrtox.c          | 12 ++++--------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 82d91547d122..cec7b1e1677a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -346,7 +346,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
 */
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
@@ -374,7 +374,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..233c7282cf5f 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -115,8 +115,7 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 {
@@ -139,8 +138,7 @@ EXPORT_SYMBOL(kstrtoull);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoll(const char *s, unsigned int base, long long *res)
 {
@@ -211,8 +209,7 @@ EXPORT_SYMBOL(_kstrtol);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtouint(const char *s, unsigned int base, unsigned int *res)
 {
@@ -242,8 +239,7 @@ EXPORT_SYMBOL(kstrtouint);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoint(const char *s, unsigned int base, int *res)
 {
-- 
2.27.0

---
 include/linux/kernel.h |  4 ++--
 lib/kstrtox.c          | 12 ++++--------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 82d91547d122..cec7b1e1677a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -346,7 +346,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
 */
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
@@ -374,7 +374,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..233c7282cf5f 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -115,8 +115,7 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 {
@@ -139,8 +138,7 @@ EXPORT_SYMBOL(kstrtoull);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoll(const char *s, unsigned int base, long long *res)
 {
@@ -211,8 +209,7 @@ EXPORT_SYMBOL(_kstrtol);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtouint(const char *s, unsigned int base, unsigned int *res)
 {
@@ -242,8 +239,7 @@ EXPORT_SYMBOL(kstrtouint);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoint(const char *s, unsigned int base, int *res)
 {
-- 
2.27.0

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Sunday, July 05, 2020 21:11 CEST, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: 
> On Sunday, July 5, 2020, Kars Mulder <kerneldev@karsmulder.nl> wrote:
> > On Saturday, July 04, 2020 22:54 CEST, Andy Shevchenko wrote:
> > > This and similar are not correct. 1/ They are not replacement per se
> > > (because of different behaviour). 2/ They simple_strto*() are not
> > > obsoleted.
> > >
> > > Can you correct all places you found and make it consistent?
> >
> > Something like the following patch? It changes all occurrences of
> > "replacement" or "obsolete" (that I'm aware of) with "preferred over".
> 
> While at it, makes sense to refer to the functions like func() as per
> kernel doc format.

So I'm working on two patches with tentative names:

[PATCH 1/2] kstrto*: Fix references to simple_strto* analogues in comments
[PATCH 2/2] kstrto*: Do not describe simple_strto* as obsolete/replaced

(I decided to switch the order since the last mail because the first
change should be less controversial than the second.)

The first one makes all simple_strto* references match their respective
functions and adds parentheses after the function name (I hope this can
happen in a single patch?), the second one removes mention of "obsolete"
or "replacement" and instead uses "preferred over".

I should probably put you in Suggested-by: for the second patch (and
maybe the first one as well?), but I need your explicit permission to
do so. So, do you want to be added as Suggested-by: on one/both of
these patches?

(Maybe I should add you as Acked-by: as well? I'm not actually sure if
it's even possible to ask you to ack the patch until its form including
changelog is finalized. Anyway, the current patches have been attached
below for good measure.)

---
 include/linux/kernel.h | 4 ++--
 lib/kstrtox.c          | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 82d91547d122..2d6050f12c64 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -346,7 +346,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Used as a replacement for the simple_strtoul(). Return code must be checked.
 */
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
@@ -374,7 +374,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Used as a replacement for the simple_strtol(). Return code must be checked.
  */
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..252ac414ba9a 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -115,7 +115,7 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
+ * Used as a replacement for the obsolete simple_strtoull(). Return code must
  * be checked.
  */
 int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
@@ -139,7 +139,7 @@ EXPORT_SYMBOL(kstrtoull);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
+ * Used as a replacement for the obsolete simple_strtoll(). Return code must
  * be checked.
  */
 int kstrtoll(const char *s, unsigned int base, long long *res)
@@ -211,7 +211,7 @@ EXPORT_SYMBOL(_kstrtol);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
+ * Used as a replacement for the obsolete simple_strtoul(). Return code must
  * be checked.
  */
 int kstrtouint(const char *s, unsigned int base, unsigned int *res)
@@ -242,7 +242,7 @@ EXPORT_SYMBOL(kstrtouint);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
+ * Used as a replacement for the obsolete simple_strtol(). Return code must
  * be checked.
  */
 int kstrtoint(const char *s, unsigned int base, int *res)
-- 
2.27.0

---
 include/linux/kernel.h |  4 ++--
 lib/kstrtox.c          | 12 ++++--------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2d6050f12c64..35fd7e0e3f04 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -346,7 +346,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoul(). Return code must be checked.
+ * Preferred over simple_strtoul(). Return code must be checked.
 */
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
@@ -374,7 +374,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtol(). Return code must be checked.
+ * Preferred over simple_strtol(). Return code must be checked.
  */
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 252ac414ba9a..a14ccf905055 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -115,8 +115,7 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull(). Return code must
- * be checked.
+ * Preferred over simple_strtoull(). Return code must be checked.
  */
 int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 {
@@ -139,8 +138,7 @@ EXPORT_SYMBOL(kstrtoull);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoll(). Return code must
- * be checked.
+ * Preferred over simple_strtoll(). Return code must be checked.
  */
 int kstrtoll(const char *s, unsigned int base, long long *res)
 {
@@ -211,8 +209,7 @@ EXPORT_SYMBOL(_kstrtol);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoul(). Return code must
- * be checked.
+ * Preferred over simple_strtoul(). Return code must be checked.
  */
 int kstrtouint(const char *s, unsigned int base, unsigned int *res)
 {
@@ -242,8 +239,7 @@ EXPORT_SYMBOL(kstrtouint);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtol(). Return code must
- * be checked.
+ * Preferred over simple_strtol(). Return code must be checked.
  */
 int kstrtoint(const char *s, unsigned int base, int *res)
 {
-- 
2.27.0

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Saturday, July 04, 2020 22:54 CEST, Andy Shevchenko wrote: 
> This and similar are not correct. 1/ They are not replacement per se
> (because of different behaviour). 2/ They simple_strto*() are not
> obsoleted.
> 
> Can you correct all places you found and make it consistent?

Something like the following patch? It changes all occurrences of
"replacement" or "obsolete" (that I'm aware of) with "preferred over".

This patch does not change the phrase "please use kstrtoul instead",
as you called that correct. I could change it to something like "prefer
using kstrtoul instead" for consistent terminology; should I?

---
 include/linux/kernel.h |  4 ++--
 lib/kstrtox.c          | 12 ++++--------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 82d91547d122..b3b10d7cc693 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -346,7 +346,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Preferred over simple_strtoul. Return code must be checked.
 */
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
@@ -374,7 +374,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the simple_strtoull. Return code must be checked.
+ * Preferred over simple_strtol. Return code must be checked.
  */
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..9be1341aaa89 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -115,8 +115,7 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoull. Return code must be checked.
  */
 int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 {
@@ -139,8 +138,7 @@ EXPORT_SYMBOL(kstrtoull);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoll. Return code must be checked.
  */
 int kstrtoll(const char *s, unsigned int base, long long *res)
 {
@@ -211,8 +209,7 @@ EXPORT_SYMBOL(_kstrtol);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtoul. Return code must be checked.
  */
 int kstrtouint(const char *s, unsigned int base, unsigned int *res)
 {
@@ -242,8 +239,7 @@ EXPORT_SYMBOL(kstrtouint);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Preferred over simple_strtol. Return code must be checked.
  */
 int kstrtoint(const char *s, unsigned int base, int *res)
 {
--
2.27.0

Re: Writing to a const pointer: is this supposed to happen?

[Andy Shevchenko]

On Sat, Jul 4, 2020 at 11:32 PM Kars Mulder <kerneldev@karsmulder.nl> wrote:
> On Saturday, July 04, 2020 16:39 CEST, Andy Shevchenko wrote:
> > > I've searched for a function that parses an int from a string and
> > > stores a pointer to the end; I can find some function simple_strtoul
> > > that matches this criterion, but it's documented as
> > >
> > >     "This function has caveats. Please use kstrtoul instead."
> > >
> > > ... and kstrtoul does not store a pointer to the end. The documentation
> > > of kstrtoul describes simple_strtoul as obsolete as well.
> >
> >
> >  Where? We need to fix that, because using simple_strto*() is fairly legal
> > in cases like this, but "has caveats".
>
> In lib/vsprintf.c, the comments before the function's implementation say:
>
>     /**
>      * simple_strtoul - convert a string to an unsigned long
>      * @cp: The start of the string
>      * @endp: A pointer to the end of the parsed string will be placed here
>      * @base: The number base to use
>      *
>      * This function has caveats. Please use kstrtoul instead.
>      */

This is correct.

> Many variants upon kstrtoul, such as kstrtoull, defined in lib/kstrtox.c,
> describe the simple_strtoull function as obsolete:
>
>     /**
>      * kstrtoull - convert a string to an unsigned long long
>      * [...]
>      *
>      * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.

>      * Used as a replacement for the obsolete simple_strtoull. Return code must
>      * be checked.
>      */

This and similar are not correct. 1/ They are not replacement per se
(because of different behaviour). 2/ They simple_strto*() are not
obsoleted.

Can you correct all places you found and make it consistent?

> I seem to have been slightly inaccurate about my claim that "kstrtoul"
> describes simple_strtoul as "obsolete" though, because in the specific
> case of kstrtoul, include/linux/kernel.h only says:
>
>     /**
>      * kstrtoul - convert a string to an unsigned long
>      * [...]
>      *
>      * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
>      * Used as a replacement for the simple_strtoull. Return code must be checked.
>     */

> (Also, there may be a documentation error here: all kstrto* functions in
> kstrtox.c and kernel.h describe themselves as replacements of simple_strtoull.
> E.g. kstrtol and kstrtoul also describe themselves as replacements of
> simple_strtoull rather than as a replacements of simple_strtol and
> simple_strtoul respectively.)
>
> By the way, I found the documentation of the caveat somewhere in
> include/linux/kernel.h:

Yes, that's what has been added lately to clarify the usage of
simple_strto*() vs. kstrto*().

>     /*
>      * Use kstrto<foo> instead.
>      *
>      * NOTE: simple_strto<foo> does not check for the range overflow and,
>      *   depending on the input, may give interesting results.
>      *
>      * Use these functions if and only if you cannot use kstrto<foo>, because
>      * the conversion ends on the first non-digit character, which may be far
>      * beyond the supported range. It might be useful to parse the strings like
>      * 10x50 or 12:21 without altering original string or temporary buffer in use.
>      * Keep in mind above caveat.
>      */


-- 
With Best Regards,
Andy Shevchenko

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Saturday, July 04, 2020 16:39 CEST, Andy Shevchenko wrote: 
> > I've searched for a function that parses an int from a string and
> > stores a pointer to the end; I can find some function simple_strtoul
> > that matches this criterion, but it's documented as
> >
> >     "This function has caveats. Please use kstrtoul instead."
> >
> > ... and kstrtoul does not store a pointer to the end. The documentation
> > of kstrtoul describes simple_strtoul as obsolete as well.
> 
> 
>  Where? We need to fix that, because using simple_strto*() is fairly legal
> in cases like this, but "has caveats".

In lib/vsprintf.c, the comments before the function's implementation say:

    /**
     * simple_strtoul - convert a string to an unsigned long
     * @cp: The start of the string
     * @endp: A pointer to the end of the parsed string will be placed here
     * @base: The number base to use
     *
     * This function has caveats. Please use kstrtoul instead.
     */

Many variants upon kstrtoul, such as kstrtoull, defined in lib/kstrtox.c,
describe the simple_strtoull function as obsolete:

    /**
     * kstrtoull - convert a string to an unsigned long long
     * [...]
     *
     * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
     * Used as a replacement for the obsolete simple_strtoull. Return code must
     * be checked.
     */

I seem to have been slightly inaccurate about my claim that "kstrtoul"
describes simple_strtoul as "obsolete" though, because in the specific
case of kstrtoul, include/linux/kernel.h only says:

    /**
     * kstrtoul - convert a string to an unsigned long
     * [...]
     *
     * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
     * Used as a replacement for the simple_strtoull. Return code must be checked.
    */

(Also, there may be a documentation error here: all kstrto* functions in
kstrtox.c and kernel.h describe themselves as replacements of simple_strtoull.
E.g. kstrtol and kstrtoul also describe themselves as replacements of
simple_strtoull rather than as a replacements of simple_strtol and
simple_strtoul respectively.)

By the way, I found the documentation of the caveat somewhere in
include/linux/kernel.h:

    /*
     * Use kstrto<foo> instead.
     *
     * NOTE: simple_strto<foo> does not check for the range overflow and,
     *	 depending on the input, may give interesting results.
     *
     * Use these functions if and only if you cannot use kstrto<foo>, because
     * the conversion ends on the first non-digit character, which may be far
     * beyond the supported range. It might be useful to parse the strings like
     * 10x50 or 12:21 without altering original string or temporary buffer in use.
     * Keep in mind above caveat.
     */

Re: Writing to a const pointer: is this supposed to happen?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

On Fri 2020-07-03 15:23:38, Kars Mulder wrote:
> > There ought to be one that returns a pointer to the first character
> > that isn't converted - but I'm no expert on the full range of these
> > functions.
> 
> I've searched for a function that parses an int from a string and
> stores a pointer to the end; I can find some function simple_strtoul
> that matches this criterion, but it's documented as
> 
>     "This function has caveats. Please use kstrtoul instead."
> 
> ... and kstrtoul does not store a pointer to the end. The documentation
> of kstrtoul describes simple_strtoul as obsolete as well. Also, there's
> no simple_strtou16 function.
> 
> It seems that the standard C function strtoul has the behaviour you
> describe as well, but this function is not defined in the kernel except
> for certain specific architectures.
> 
> > The problem with strdup() is you get the extra (unlikely) failure path.
> > 128 bytes of stack won't be a problem if the function is (essentially)
> > a leaf.
> > Deep stack use is actually likely to be in the bowels of printf())
> > inside an obscure error path.
> 
> The function already makes a call to kcalloc, so the unlikely out-of-
> memory error path already exists; a second memory allocation just
> makes it slightly less unlikely. The two new out-of-memory conditions
> do happen at different points of the function though, making them
> have different side effects. I could fix this by moving my code.
> 
> As for this function being a leaf: keep in mind that this function has
> the potential of calling printk in an obscure error condition (the user-
> provided parameter being longer that 128 characters); quirks_param_set
> calls param_set_copystring, which on its turn calls pr_err, which is a
> macro for printk.
> 
> Meanwhile, here's a patch for copying the parameter to the stack:

Looks good, I guess Signed-off-by would be useful.

								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

RE: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

> There ought to be one that returns a pointer to the first character
> that isn't converted - but I'm no expert on the full range of these
> functions.

I've searched for a function that parses an int from a string and 
stores a pointer to the end; I can find some function simple_strtoul
that matches this criterion, but it's documented as

    "This function has caveats. Please use kstrtoul instead."

... and kstrtoul does not store a pointer to the end. The documentation
of kstrtoul describes simple_strtoul as obsolete as well. Also, there's
no simple_strtou16 function.

It seems that the standard C function strtoul has the behaviour you
describe as well, but this function is not defined in the kernel except
for certain specific architectures.

> The problem with strdup() is you get the extra (unlikely) failure path.
> 128 bytes of stack won't be a problem if the function is (essentially)
> a leaf.
> Deep stack use is actually likely to be in the bowels of printf())
> inside an obscure error path.

The function already makes a call to kcalloc, so the unlikely out-of-
memory error path already exists; a second memory allocation just
makes it slightly less unlikely. The two new out-of-memory conditions
do happen at different points of the function though, making them
have different side effects. I could fix this by moving my code.

As for this function being a leaf: keep in mind that this function has
the potential of calling printk in an obscure error condition (the user-
provided parameter being longer that 128 characters); quirks_param_set
calls param_set_copystring, which on its turn calls pr_err, which is a
macro for printk.

Meanwhile, here's a patch for copying the parameter to the stack:

---
 drivers/usb/core/quirks.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..86b1a6739b4e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -12,6 +12,8 @@
 #include <linux/usb/hcd.h>
 #include "usb.h"
 
+#define QUIRKS_PARAM_SIZE 128
+
 struct quirk_entry {
 	u16 vid;
 	u16 pid;
@@ -23,19 +25,21 @@ static DEFINE_MUTEX(quirk_mutex);
 static struct quirk_entry *quirk_list;
 static unsigned int quirk_count;
 
-static char quirks_param[128];
+static char quirks_param[QUIRKS_PARAM_SIZE];
 
-static int quirks_param_set(const char *val, const struct kernel_param *kp)
+static int quirks_param_set(const char *value, const struct kernel_param *kp)
 {
+	char val[QUIRKS_PARAM_SIZE];
 	char *p, *field;
 	u16 vid, pid;
 	u32 flags;
 	size_t i;
 	int err;
 
-	err = param_set_copystring(val, kp);
+	err = param_set_copystring(value, kp);
 	if (err)
 		return err;
+	strscpy(val, value, sizeof(val));
 
 	mutex_lock(&quirk_mutex);
 
--
2.27.0

RE: Writing to a const pointer: is this supposed to happen?

[David Laight]

From: Kars Mulder
> Sent: 02 July 2020 22:48
> 
> On Thursday, July 02, 2020 09:55 CEST, David Laight wrote:
> > Hmm... sscanf() is also horrid.
> > Surprisingly difficult to use correctly.
> >
> > It is usually best to use strchr() (and maybe str[c]scn())
> > to parse strings.
> > For numbers use whatever the kernels current 'favourite' implementation
> > of strtoul() is called.
> 
> I thought that using sscanf would clean up the code a bit compared to
> several haphazard calls, but I can see your point about sscanf being
> difficult to use correctly.
> 
> The kernel functions kstrtou16 seem to expect a null-terminated string
> as argument. Since there are no null-bytes after the numbers we want to
> parse, it becomes necessary to copy at least part of the strings to a
> buffer.

There ought to be one that returns a pointer to the first character
that isn't converted - but I'm no expert on the full range of these
functions.

> If we're copying strings to buffers anyway, I think the simplest
> solution would be to just kstrdup the entire parameter and not touch
> the rest of the string parsing code. This has the disadvantage of
> having an extra memory allocation to keep track of.
> 
> Since the parameter is currently restricted to 128 characters at
> most, it may alternatively be possible to copy the parameter to
> a 128-byte buffer on the stack. This has the advantage of having
> to keep track of one less memory allocation, but the disadvantage
> of using 128 bytes more stack space; I'm not sure whether that's
> acceptable.

The problem with strdup() is you get the extra (unlikely) failure path.
128 bytes of stack won't be a problem if the function is (essentially)
a leaf.
Deep stack use is actually likely to be in the bowels of printf())
inside an obscure error path.
Many years ago (about 1984) I parsed the object code of a program
to find the deepest stack use (no recursion and no function pointers)
so we could set the stack sizes correctly - there wasn't enough
memory to do it properly!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Thursday, July 02, 2020 09:55 CEST, David Laight wrote: 
> Hmm... sscanf() is also horrid.
> Surprisingly difficult to use correctly.
> 
> It is usually best to use strchr() (and maybe str[c]scn())
> to parse strings.
> For numbers use whatever the kernels current 'favourite' implementation
> of strtoul() is called.

I thought that using sscanf would clean up the code a bit compared to
several haphazard calls, but I can see your point about sscanf being
difficult to use correctly.

The kernel functions kstrtou16 seem to expect a null-terminated string
as argument. Since there are no null-bytes after the numbers we want to
parse, it becomes necessary to copy at least part of the strings to a
buffer.

If we're copying strings to buffers anyway, I think the simplest
solution would be to just kstrdup the entire parameter and not touch
the rest of the string parsing code. This has the disadvantage of
having an extra memory allocation to keep track of.

Since the parameter is currently restricted to 128 characters at
most, it may alternatively be possible to copy the parameter to
a 128-byte buffer on the stack. This has the advantage of having
to keep track of one less memory allocation, but the disadvantage
of using 128 bytes more stack space; I'm not sure whether that's
acceptable.

Here's a sample patch involving kstrdup:

---
 drivers/usb/core/quirks.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..3b64b0be2563 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -25,17 +25,23 @@ static unsigned int quirk_count;
 
 static char quirks_param[128];
 
-static int quirks_param_set(const char *val, const struct kernel_param *kp)
+static int quirks_param_set(const char *value, const struct kernel_param *kp)
 {
-	char *p, *field;
+	char *val, *p, *field;
 	u16 vid, pid;
 	u32 flags;
 	size_t i;
 	int err;
 
+	val = kstrdup(value, GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
 	err = param_set_copystring(val, kp);
-	if (err)
+	if (err) {
+		kfree(val);
 		return err;
+	}
 
 	mutex_lock(&quirk_mutex);
 
@@ -60,6 +66,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 	if (!quirk_list) {
 		quirk_count = 0;
 		mutex_unlock(&quirk_mutex);
+		kfree(val);
 		return -ENOMEM;
 	}
 
@@ -144,6 +151,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 
 unlock:
 	mutex_unlock(&quirk_mutex);
+	kfree(val);
 
 	return 0;
 }
--
2.27.0

RE: Writing to a const pointer: is this supposed to happen?

[David Laight]

From: Kars Mulder
> Sent: 02 July 2020 00:03
> On Saturday, June 27, 2020 12:24 CEST, David Laight wrote:
> > The code quoted (using strset()) is almost certainly wrong.
> > The caller is unlikely to expect the input be modified.
> > Since it doesn't fault the string must be in read-write memory.
> 
> I tried writing a patch that avoids the writing-to-const-pointer issue
> by using the less intrusive sscanf function instead of strsep. It might
> avoid a potential bug when somebody wrongly assumes that a
> kernel_param_ops.set function will not write to its const char* argument.

Hmm... sscanf() is also horrid.
Surprisingly difficult to use correctly.

It is usually best to use strchr() (and maybe str[c]scn())
to parse strings.
For numbers use whatever the kernels current 'favourite' implementation
of strtoul() is called.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Saturday, June 27, 2020 12:24 CEST, David Laight wrote: 
> The code quoted (using strset()) is almost certainly wrong.
> The caller is unlikely to expect the input be modified.
> Since it doesn't fault the string must be in read-write memory.

I tried writing a patch that avoids the writing-to-const-pointer issue
by using the less intrusive sscanf function instead of strsep. It might
avoid a potential bug when somebody wrongly assumes that a
kernel_param_ops.set function will not write to its const char* argument.

Would a patch like this be acceptable, or would I first have to
demonstrate that the current implementation is actually causing real
problems?

This is not yet a formal patch submission; I have some more rigorous
testing to do first. (Relatedly, I'm a bit confused by the requirement
to "always *test* your changes on at least 4 or 5 people" mentioned in
MAINTAINERS. Where should I find those people? Can those people come
from the LKML mailing list, or should I find testers on some third party
forum before submitting my patch to the LKML?)

---
 drivers/usb/core/quirks.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..fe2059d71705 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -27,11 +27,11 @@ static char quirks_param[128];
 
 static int quirks_param_set(const char *val, const struct kernel_param *kp)
 {
-	char *p, *field;
-	u16 vid, pid;
+	const char *p;
+	unsigned short vid, pid;
 	u32 flags;
-	size_t i;
-	int err;
+	size_t i, len;
+	int err, res;
 
 	err = param_set_copystring(val, kp);
 	if (err)
@@ -63,29 +63,16 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 		return -ENOMEM;
 	}
 
-	for (i = 0, p = (char *)val; p && *p;) {
+	for (i = 0, p = val; p && *p;) {
 		/* Each entry consists of VID:PID:flags */
-		field = strsep(&p, ":");
-		if (!field)
-			break;
-
-		if (kstrtou16(field, 16, &vid))
-			break;
-
-		field = strsep(&p, ":");
-		if (!field)
-			break;
-
-		if (kstrtou16(field, 16, &pid))
-			break;
-
-		field = strsep(&p, ",");
-		if (!field || !*field)
+		res = sscanf(p, "%hx:%hx%zn", &vid, &pid, &len);
+		if (res != 2 || *(p+len) != ':')
 			break;
+		p += len+1;
 
 		/* Collect the flags */
-		for (flags = 0; *field; field++) {
-			switch (*field) {
+		for (flags = 0; *p; p++) {
+			switch (*p) {
 			case 'a':
 				flags |= USB_QUIRK_STRING_FETCH_255;
 				break;
@@ -132,11 +119,15 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 				flags |= USB_QUIRK_HUB_SLOW_RESET;
 				break;
 			/* Ignore unrecognized flag characters */
+			case ',':
+				p++;
+				goto collect_flags_end;
 			}
 		}
+collect_flags_end:
 
 		quirk_list[i++] = (struct quirk_entry)
-			{ .vid = vid, .pid = pid, .flags = flags };
+			{ .vid = (u16)vid, .pid = (u16)pid, .flags = flags };
 	}
 
 	if (i < quirk_count)
--
2.27.0

RE: Writing to a const pointer: is this supposed to happen?

[David Laight]

From: Kars Mulder
> Sent: 24 June 2020 16:26
> On Wednesday, June 24, 2020 15:10 CEST, Greg Kroah-Hartman wrote:
> > Have you hit any runtime issues with this code doing this?  These
> > strings should be held in writable memory, right?  Or do you see a
> > codepath where that is not the case?
> 
> I haven't ran into any issues with it; I was just looking at the code
> as reference for a patch I'm working on.
> 
> I initially assumed that casting a const pointer to non-const and
> writing to it would be undefined behaviour, but after reading through
> the C99 standard I can't find an unambiguous statement whether it is
> undefined behaviour even if the const pointer points to an object that
> was originally non-const (like char* -> const char* -> char* casts); it
> only says it is undefined behaviour if the object was defined with the
> const qualifier.
> 
> I should probably stop bothering you with my newbie questions.

IISC The C standard 'rules' about casts only really allow a pointer to
be temporarily cast to a different type and then cast back to the
original type before being used.

One effect of that is code like:
void foo(bah_t *bahp)
{
	bah_t bah;
	/* Copy because bahp might be misaligned */
	memcpy(&bah, (void *)bahp, sizeof bah);
doesn't work because the compiler knows that 'bahp' must
point to aligned memory - because that is the only place
it can legitimately come from.
No amount of casts will stop it inlining memcpy() and using
wide register copier.

The code quoted (using strset()) is almost certainly wrong.
The caller is unlikely to expect the input be modified.
Since it doesn't fault the string must be in read-write memory.

Code can be compiled with -Wcast-qual that will generate a
warning when 'const' gets cast away.
There are some legitimate reasons to remove 'const', but not
many.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Wednesday, June 24, 2020 15:10 CEST, Greg Kroah-Hartman wrote: 
> Have you hit any runtime issues with this code doing this?  These
> strings should be held in writable memory, right?  Or do you see a
> codepath where that is not the case?

I haven't ran into any issues with it; I was just looking at the code
as reference for a patch I'm working on.

I initially assumed that casting a const pointer to non-const and
writing to it would be undefined behaviour, but after reading through
the C99 standard I can't find an unambiguous statement whether it is
undefined behaviour even if the const pointer points to an object that
was originally non-const (like char* -> const char* -> char* casts); it
only says it is undefined behaviour if the object was defined with the
const qualifier.

I should probably stop bothering you with my newbie questions.

Re: Writing to a const pointer: is this supposed to happen?

[Greg Kroah-Hartman]

On Wed, Jun 24, 2020 at 02:34:45PM +0200, Kars Mulder wrote:
> On Tuesday, June 23, 2020 21:55 CEST, Pavel Machek wrote:
> > Odd, indeed... but not likely to cause immediate problems.
> >
> > You may want to cc relevant maintainers, or even run git
> > blame and contact author.
> 
> Thank you for your response.
> 
> The code was written by Kai-Heng Feng, whom I shall CC. The code is
> part of the usbcore module, which does not have a maintainer listed in
> MAINTAINERS, but the patch and most other recent patches to usbcore
> were signed off exclusively by Greg Kroah-Hartman, so I guess that
> makes him the de facto maintainer? I'll CC him as well.
> 
> I'm not sure whether it is easy to read the previous messages of this
> thread if you got CC'ed just now, so I'll repeat/paraphrase the
> important part of my initial mail for your convenience:
> 
> > In the file drivers/usb/core/quirks.c, I noticed that the function
> > quirks_param_set writes to a const pointer, and would like to check
> > whether this is ok with the kernel programming practices. Here are
> > the relevant lines from the function (several lines omitted):
> >
> > 	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
> > 		char *p, *field;
> > 		for (i = 0, p = (char *)val; p && *p;) {
> > 			field = strsep(&p, ":");
> >
> > In here a const pointer *val is cast into a non-const pointer and
> > then written to by the function strsep, which replaces the first
> > occurrence of the ':' token with a null-byte. Is this allowed?
> 
> CC: Kai-Heng Feng <kai.heng.feng@canonical.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

It's not the nicest thing, but as you have noticed, it all works just
fine, right?

Have you hit any runtime issues with this code doing this?  These
strings should be held in writable memory, right?  Or do you see a
codepath where that is not the case?  If so, please feel free to submit
a patch to fix this up (and probably fix up a number of other "set"
functions that deal with struct kernel_param as well.)

thanks,

greg k-h

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Tuesday, June 23, 2020 21:55 CEST, Pavel Machek wrote: 
> Odd, indeed... but not likely to cause immediate problems.
> 
> You may want to cc relevant maintainers, or even run git
> blame and contact author.

Thank you for your response.

The code was written by Kai-Heng Feng, whom I shall CC. The code is
part of the usbcore module, which does not have a maintainer listed in
MAINTAINERS, but the patch and most other recent patches to usbcore
were signed off exclusively by Greg Kroah-Hartman, so I guess that
makes him the de facto maintainer? I'll CC him as well.

I'm not sure whether it is easy to read the previous messages of this
thread if you got CC'ed just now, so I'll repeat/paraphrase the
important part of my initial mail for your convenience:

> In the file drivers/usb/core/quirks.c, I noticed that the function
> quirks_param_set writes to a const pointer, and would like to check
> whether this is ok with the kernel programming practices. Here are
> the relevant lines from the function (several lines omitted):
> 
> 	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
> 		char *p, *field;
> 		for (i = 0, p = (char *)val; p && *p;) {
> 			field = strsep(&p, ":");
>
> In here a const pointer *val is cast into a non-const pointer and
> then written to by the function strsep, which replaces the first
> occurrence of the ':' token with a null-byte. Is this allowed?

CC: Kai-Heng Feng <kai.heng.feng@canonical.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: Writing to a const pointer: is this supposed to happen?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Mon 2020-06-22 13:35:35, Kars Mulder wrote:
> In the file drivers/usb/core/quirks.c, I noticed a couple of odd things about the function "quirks_param_set", and I'd like to check whether those are ok according to the kernel programming practices. Here are the relevant lines from the function (several lines omitted):
> 
> 	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
> 		char *p, *field;
> 		for (i = 0, p = (char *)val; p && *p;) {
> 			field = strsep(&p, ":");
> 			if (!field)
> 				break;
> 
> In here a const pointer *val is cast into a non-const pointer and then written to by the function strsep, which replaces the first occurrence of the ':' token by a null-byte. Is this allowed?
> 

Odd, indeed... but not likely to cause immediate problems.

You may want to cc relevant maintainers, or even run git
blame and contact author.

You may also want to wrap your lines at 72 characters or so.

Best regards,
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

In the file drivers/usb/core/quirks.c, I noticed a couple of odd things about the function "quirks_param_set", and I'd like to check whether those are ok according to the kernel programming practices. Here are the relevant lines from the function (several lines omitted):

	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
		char *p, *field;
		for (i = 0, p = (char *)val; p && *p;) {
			field = strsep(&p, ":");
			if (!field)
				break;

In here a const pointer *val is cast into a non-const pointer and then written to by the function strsep, which replaces the first occurrence of the ':' token by a null-byte. Is this allowed?

On a minor side note, this function immediately checks whether the first call to strsep(&p, ":") returned a nullpointer. From what I can learn from the documentation, strsep always returns what *&p was when the strsep was called, and p is verified to be nonzero in the loop condition right before the call to strsep. Is this check actually necessary? Is it a good idea to add a return-value check anyway even if it is not necessary, as an abundance of caution?