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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Sep 11, 2020 at 12:53:55PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote:
> > On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote:
> > > 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:
> 
> ...
> 
> > > > > +#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
> > > 
> > > ?
> > > 
> > 
> > Pretty much, though I was suggesting keeping it in a helper function,
> > say lineevent_user_size(), not in lineevent_read() itself, to isolate
> > all the ugliness in one small place.
> > 
> > So in lineevent_read() you would:
> > 
> >    size_t ge_size = lineevent_user_size();
> > 
> > and then use that to replace all the sizeof(ge) occurrences.
> 
> But in any case it makes code a bit hackish, no?
> 

Maybe - I'm not totally happy about copying a partial struct either, but
one way or the other that is what you effectively need to do.

> We calculate the size of one structure and by the fact *partially* copy
> another one.
> 

Perhaps it is a matter of perspective - as I see it you are calculating
the length of the event that the userspace expects, and then copying only
that amount.
As you say in the comment "i386 has no padding after 'id'" - other than
that it is bitwise identical - the only difference is the length.

> And actually if you look closer to the types, the compat_u64 is not the same as u64.
> So, I'm not sure your solution would work in all cases.
> 

There is only one corner case here - 32-bit on x86_64, and it works for
that - I've tested it.
For all other cases it falls back to the full length.

For x86 the compat_u64 is:

typedef u64 __attribute__((aligned(4))) compat_u64;

which is bitwise identical - only allowed to 32-bit align.
And if compat_u64 wasn't bitwise identical in this case then your original
code wouldn't work either ;-).

Cheers,
Kent.