Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote:
> On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:
> > The introduced line even handling ABI in the commit
> > 
> 
> s/even/event/
> 
> >   61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> > 
> > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > In such case the very first check in the lineevent_read() will fail
> > due to alignment differences.
> > 
> > To workaround this introduce lineeven_to_user() helper which returns actual
> > size of the structure and copies its content to user if asked.
> > 
> 
> And again here.

Thanks!

...

> > +#ifdef __x86_64__
> > +	/* i386 has no padding after 'id' */
> > +	if (in_ia32_syscall()) {
> > +		struct compat_ge {
> > +			compat_u64	timestamp __packed;
> > +			u32		id;
> > +		} cge;
> > +
> > +		if (buf && ge) {
> > +			cge = (struct compat_ge){ ge->timestamp, ge->id };
> > +			if (copy_to_user(buf, &cge, sizeof(cge)))
> > +				return -EFAULT;
> > +		}
> > +
> > +		return sizeof(cge);
> > +	}
> > +#endif
> > +
> > +	if (buf && ge) {
> > +		if (copy_to_user(buf, ge, sizeof(*ge)))
> > +			return -EFAULT;
> > +	}
> > +
> > +	return sizeof(*ge);
> > +}
> >  
> 
> The dual-purpose nature of this function makes it more complicated than
> it needs to be.
> I was going to suggest splitting it into separate functions, but...
> 
> Given that struct compat_ge is a strict truncation of struct
> gpioevent_data, how about reducing this function to just determining the
> size of the event for user space, say lineevent_user_size(), and
> replacing sizeof(ge) with that calculcated size throughout
> lineevent_read()?

So you mean something like

	struct compat_gpioeevent_data {
		compat_u64	timestamp;
		u32		id;
	} __packed;

#ifdef __x86_64__
	/* i386 has no padding after 'id' */
	size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data);
#else
	size_t ge_size = sizeof(struct gpioevent_data) ;
#endif

?

...

> Is patch 2 in any way related to this patch?

No. It can be applied separately.

-- 
With Best Regards,
Andy Shevchenko