On Mon, Feb 27, 2017 at 12:02 PM, Albert ARIBAUD albert.aribaud@3adev.fr wrote:
Hi Arnd,
On Thu, 23 Feb 2017 21:24:22 +0100, Arnd Bergmann arnd@arndb.de wrote :
On Thu, Feb 23, 2017 at 6:31 PM, Joseph Myers joseph@codesourcery.com wrote:
On Thu, 23 Feb 2017, Arnd Bergmann wrote:
[...]
I had thought of that as well, but wasn't sure if we could rely on bitfields to do the right thing across C standard versions and non-gcc compilers. I experimentally found that gcc-4.9 and higher accept that version without a -Wpedantic warning when using --std=c99 or --std=c11, but they warn about it with --std=c89 or --std=gnu89. Older gcc versions as far back as gcc-3.4 (the oldest I could try) always warn about anonymous bitfields with -pedantic, but I could still silence that warning using the __extension__ keyword. I think that's good enough.
What's the warning you see, with what exact testcase? Unnamed bit-fields (provided the declared type is int / signed int / unsigned int, not any other integer or enum type), and initializers ignoring them, date back at least to the third public review draft of C89 (13 May 1988), which is the oldest version I have to hand.
Sorry, my mistake, I used "long : 32", which causes "warning: type of bit-field '<anonymous>' is a GCC extension [-Wpedantic]". With 'int' it works fine.
So IIUC (and assuming we keep tv_nsec a long in APIs) we /could/ pad struct timespec with an anonymous bitfield and still be compatible with (most) existing user source code, and it is considered acceptable (but should be duly documented). Added this in rev. 122of the design doc.
This solves the problem in user space, but it still leaves the other problem that Zack and I mentioned on the user/kernel boundary: we have to zero out the padding since at least 64-bit kernels with 32-bit user space would interpret the padding as nanoseconds over 1 billion. We may also want to make the kernel behavior consistent between 32-bit and 64-bit kernels, so the kernel will either always ignore the padding (which is a bit more work for the kernel) or it will always reject nonzero upper bits of the nanoseconds (as 64-bit kernels do today for native tasks, this means more work in 32-bit glibc, or using 64-bit nanoseconds as Zack suggested).
Right now, I have this in the kernel, to do the zeroing whenever we copy a __kernel_timespec from user space into a timespec64 in the kernel:
typedef __s64 __kernel_time64_t;
struct __kernel_timespec { __kernel_time64_t tv_sec; __s64 tv_nsec; };
int get_timespec64(struct timespec64 *ts, const struct __kernel_timespec __user *uts) { struct __kernel_timespec kts; int ret;
ret = copy_from_user(&kts, uts, sizeof(kts)); if (ret) return -EFAULT;
ts->tv_sec = kts.tv_sec; if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall()) kts.tv_nsec &= 0xfffffffful; ts->tv_nsec = kts.tv_nsec;
return 0; }
This should catch anything that passes in a timespec by itself (clock_settime, clock_nanosleep, timer_settime, timerfd_settime, ppoll, pselect6, io_getevents, recvmmsg, mq_timedsend, mq_timedreceive, semtimedop, utimensat, rt_sigtimedwait, futex and some ioctls), but not structures that embed a timespec (fortunately no syscalls, very few ioctls). The external function call plus the in_compat_syscall() check will add a couple of cycles on 64-bit architectures, for both 32-bit and 64-bit tasks. We could optimize this slightly using
if (IS_ENABLED(CONFIG_64BIT)) kts.tv_nsec &= current->tv_nsec_mask;
Arnd