Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Alexey Dobriyan <adobriyan@gmail.com>]

On Sat, Aug 29, 2015 at 12:43:04PM +0200, Dongsu Park wrote:
> > How about adding a for-purpose string-to-uid/gid function, rather than
> > open-coding?

>  		case Opt_uid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			uid = make_kuid(current_user_ns(), option);
> -			if (!uid_valid(uid))
> -				return -EINVAL;
> +			rc = kstrtouid(args[0].from, &uid);
> +			if (rc)
> +				return rc;
> +
>  			opts->uid = uid;

if kstrtouid is doing all the work, value should be put directly into
opts->uid avoiding temporary variables.

>  		case Opt_gid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			gid = make_kgid(current_user_ns(), option);
> -			if (!gid_valid(gid))
> -				return -EINVAL;
> +			rc = kstrtogid(args[0].from, &gid);
> +			if (rc)
> +				return rc;

ditto

> +
>  			opts->gid = gid;
>  			opts->setgid = 1;
>  			break;

> --- a/include/linux/parse-integer.h
> +++ b/include/linux/parse-integer.h
> @@ -2,6 +2,7 @@
>  #define _PARSE_INTEGER_H
>  #include <linux/compiler.h>
>  #include <linux/types.h>
> +#include <linux/uidgid.h>

no, just put the prototypes into uidgid.h
This header is supposed to be small and self contained.

> @@ -155,6 +156,9 @@ static inline int __must_check kstrtos8(const char *s, unsigned int base, s8 *re
>  	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
>  }
>  
> +int __must_check kstrtouid(const char *uidstr, kuid_t *kuid);
> +int __must_check kstrtogid(const char *gidstr, kgid_t *kgid);
> +
>  int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
>  int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
>  int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);

> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -17,6 +17,8 @@
>  #include <linux/math64.h>
>  #include <linux/export.h>
>  #include <linux/types.h>
> +#include <linux/sched.h>
> +#include <linux/cred.h>

This is pretty ridiculous for what is supposed to be bunch  of integer
conversion routines. sched.h? come on!

> @@ -81,6 +83,44 @@ int f(const char __user *s, size_t count, unsigned int base, type *res)	\
>  }									\
>  EXPORT_SYMBOL(f)
>  
> +int kstrtouid(const char *uidstr, kuid_t *kuid)
> +{
> +	uid_t uidval;
> +	kuid_t kuidtmp;
> +
> +	int rc = kstrtouint(uidstr, 0, &uidval);
> +
> +	if (rc)
> +		return rc;

and please follow coding style.

> +
> +	kuidtmp = make_kuid(current_user_ns(), uidval);
> +	if (!uid_valid(kuidtmp))
> +		return -EINVAL;
> +
> +	*kuid = kuidtmp;
> +	return 0;
> +}
> +EXPORT_SYMBOL(kstrtouid);
> +
> +int kstrtogid(const char *gidstr, kgid_t *kgid)
> +{
> +	gid_t gidval;
> +	kgid_t kgidtmp;
> +
> +	int rc = kstrtouint(gidstr, 0, &gidval);
> +
> +	if (rc)
> +		return rc;
> +
> +	kgidtmp = make_kgid(current_user_ns(), gidval);
> +	if (!gid_valid(kgidtmp))
> +		return -EINVAL;
> +
> +	*kgid = kgidtmp;
> +	return 0;
> +}
> +EXPORT_SYMBOL(kstrtogid);