Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Takashi Iwai <tiwai@suse.de>]

On Tue, 01 Aug 2023 19:51:39 +0200,
Andy Shevchenko wrote:
> 
> On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:
> > On Mon, 31 Jul 2023 21:40:20 +0200,
> > Mark Brown wrote:
> > > On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> > > > Mark Brown wrote:
> > > 
> > > > > It really feels like we ought to rename, or add an alias for, the type
> > > > > if we're going to start using it more widely - it's not helping to make
> > > > > the code clearer.
> > > 
> > > > That was my very first impression, too, but I changed my mind after
> > > > seeing the already used code.  An alias might work, either typedef or
> > > > define genptr_t or such as sockptr_t.  But we'll need to copy the
> > > > bunch of helper functions, too...
> > > 
> > > I would predict that if the type becomes more widely used that'll happen
> > > eventually and the longer it's left the more work it'll be.
> > 
> > That's true.  The question is how more widely it'll be used, then.
> > 
> > Is something like below what you had in mind, too?
> 
> I agree with your proposal (uniptr_t also works for me), but see below.
> 
> ...
> 
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> 
> But let make the list of the headers right this time.
> 
> It seems to me that
> 
> err.h
> minmax.h // maybe not, see a remark at the bottom
> string.h
> types.h
> 
> are missing.

OK, makes sense to add them.

> 
> More below.
> 
> ...
> 
> > +	void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
> > +
> > +	if (!p)
> > +		return ERR_PTR(-ENOMEM);
> 
> This can use cleanup.h.

Hm, I don't think it can be replaced with that.
There may be more use of cleanup.h, but it's no direct alternative for
kmalloc_track_caller()...

> > +	if (copy_from_uniptr(p, src, len)) {
> > +		kfree(p);
> > +		return ERR_PTR(-EFAULT);
> > +	}
> > +	return p;
> > +}
> > +
> > +static inline void *memdup_uniptr_nul(uniptr_t src, size_t len)
> > +{
> > +	char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);
> 
> Ditto.
> 
> > +	if (!p)
> > +		return ERR_PTR(-ENOMEM);
> > +	if (copy_from_uniptr(p, src, len)) {
> > +		kfree(p);
> > +		return ERR_PTR(-EFAULT);
> > +	}
> > +	p[len] = '\0';
> > +	return p;
> > +}
> 
> ...
> 
> > +static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count)
> > +{
> > +	if (uniptr_is_kernel(src)) {
> > +		size_t len = min(strnlen(src.kernel, count - 1) + 1, count);
> 
> I didn't get why do we need min()? To check the count == 0 case?
> 
> Wouldn't
> 
> 		size_t len;
> 
> 		len = strnlen(src.kernel, count);
> 		if (len == 0)
> 			return 0;
> 
> 		/* Copy a trailing NUL if found */
> 		if (len < count)
> 			len++;
> 
> be a good equivalent?

A good question.  I rather wonder why it can't be simple strncpy().

> > +		memcpy(dst, src.kernel, len);
> > +		return len;
> > +	}
> > +	return strncpy_from_user(dst, src.user, count);
> > +}
> 
> ...
> 
> > +static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t size)
> > +{
> > +	if (!uniptr_is_kernel(src))
> 
> Why not to align all the functions to use same conditional (either always
> positive or negative)?

A different person, a different taste :)  But it's trivial to fix.

> > +		return check_zeroed_user(src.user + offset, size);
> > +	return memchr_inv(src.kernel + offset, 0, size) == NULL;
> > +}
> 
> ...
> 
> Taking all remarks into account I would rather go with sockptr.h being
> untouched for now, just a big
> 
> /* DO NOT USE, it's obsolete, use uniptr.h instead! */
> 
> to be added.

Possibly that's a safer choice.  But the biggest question is whether
we want a generic type or not.  Let's try to ask it first.

Interestingly, this file doesn't belong to any subsystem in
MAINTAINERS, so I'm not sure who to be Cc'ed.  Chirstoph as the
original author and net dev, maybe.


thanks,

Takashi