Misbehavior with setsockopt timeval structure with -fpack-struct enabled

Misbehavior with setsockopt timeval structure with -fpack-struct enabled

[Drew B.]

Hi everyone!
I've got a very strange behavior on Linux and OS X build of the same 
source code. To be specific, when I try to set socket timeout option:

...
struct timeval timeout;
timeout.tv_sec = 0;
timeout.tv_usec = 1;
ret = setsockopt(so, SOL_SOCKET, SO_RCVTIMEO, &timeout, 
sizeof(timeout));
...

with -fpack-struct enabled, on Linux machine the size of timeval struct 
is 16 bytes (as well as unpacked), while on OS X it's 12 for packed and 
16 for unpacked. In which case I get an error while trying to apply the 
setting to the socket. I dug a little bit deeper and the following piece 
of code in net/core/sock.c:

...
int sock_copy_user_timeval(struct __kernel_sock_timeval *tv,
			   sockptr_t optval, int optlen, bool old_timeval)
{
	if (old_timeval && in_compat_syscall() && !COMPAT_USE_64BIT_TIME) {
		struct old_timeval32 tv32;

		if (optlen < sizeof(tv32))
			return -EINVAL;

		if (copy_from_sockptr(&tv32, optval, sizeof(tv32)))
			return -EFAULT;
		tv->tv_sec = tv32.tv_sec;
		tv->tv_usec = tv32.tv_usec;
	} else if (old_timeval) {
		struct __kernel_old_timeval old_tv;

		if (optlen < sizeof(old_tv))
			return -EINVAL;
		if (copy_from_sockptr(&old_tv, optval, sizeof(old_tv)))
			return -EFAULT;
		tv->tv_sec = old_tv.tv_sec;
		tv->tv_usec = old_tv.tv_usec;
	} else {
		if (optlen < sizeof(*tv))
			return -EINVAL;
		if (copy_from_sockptr(tv, optval, sizeof(*tv)))
			return -EFAULT;
	}

	return 0;
}
EXPORT_SYMBOL(sock_copy_user_timeval);
...

So, to be specific (same or similar logics goes through the function in 
respective places):
if (optlen < sizeof(tv32))
	return -EINVAL;
Which means, that it doesn't consider whether the structure is packed or 
not, it always compares against unpacked (?) structure == 16 bytes (for 
now).

Is it expected?

Kind regards,
Drew.

Re: Misbehavior with setsockopt timeval structure with -fpack-struct enabled

[Drew B.]

Hi David!
Thanks for your answer. Just to feed my curiosity, why? Of course I 
could use:
#pragma pack(1, push)
...
#pragma pack(pop)
to pack only what is needed, but in the first place I was thinking about 
keeping as much free memory as possible (to make things optimized). And 
keep other things intact, but is it not a good practice placing 
-fpack-struct into compile-time params?

Kind regards,
Drew.

On 2023-07-19 13:36, David Laight wrote:
> ...
>> with -fpack-struct enabled,
> 
> Don't even think of enabling that.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, 
> MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: Misbehavior with setsockopt timeval structure with -fpack-struct enabled

[Drew B.]

Hi David!

>> #pragma pack(1, push)
>> ...
>> #pragma pack(pop)
> That is M$ C :-)
> For gcc you can use __attribute__((packed))
Noted. I new about things like __attribute__ ((unused)), but forgot 
about mentioned above. My thanks!

> You also pretty much never, ever, want to 'pack' a structure
> unless you need to match a 'hardware/protocol structure' that
> has fields that aren't on their natural boundaries.
Straight to the point! That was the reason why I "packed" the things :).

> Everything that uses a structure has to use the same alignment.
> So 'randomly' packing system structures will break things.
And by 'randomly' you mean using the gcc param instead of attribute 'in 
place'?

> If you need to make structures portable between architectures
> then add explicit padding to ensure 64bit items are on their
> natural boundaries (as well as byteswapping as necessary).
Frankly speaking, the size of the data is relatively small. And 
byteswapping thing is resolved through the union and byte array, so 
everything is being sent as a bytearray of known size and then the type 
casting thing happens based on the header information.

Kind regards,
Drew.