I'm adding a couple ioctls to valgrind. Here's part of the declaration in include/vki/vki-linux.h: #define VKI_FS_IOC_FIEMAP _VKI_IOWR('f', 11, struct vki_fiemap) #define VKI_OBD_IOC_FID2PATH _VKI_IOWR ('f', 150, OBD_IOC_DATA_TYPE) The first one works fine, but not the second. To understand, I've added the following trace at the very top of PRE(sys_ioctl) and POST(sys_ioctl): VG_(printf)("sys_ioctl POST ( %lx, %lx, %lx )\n", ARG2, VKI_FS_IOC_FIEMAP, VKI_OBD_IOC_FID2PATH); and here's the output for both ioctls: sys_ioctl PRE ( c020660b, c020660b, c0086696 ) sys_ioctl POST ( c020660b, c020660b, c0086696 ) ... sys_ioctl PRE ( ffffffffc0086696, c020660b, c0086696 ) sys_ioctl POST ( ffffffffc0086696, c020660b, c0086696 ) So for some reason, a sign extension happens in ARG2 for the VKI_OBD_IOC_FID2PATH opcode, but not the other. Since the opcode is a 32 bits value given to ioctl(), I suspect something in valgrind is expanding the sign of ARG2. The bug is easily reproducible on my setup, but needs a working Lustre Filesystem to reproduce the OBD_IOC_FID2PATH ioctl. Reproducible: Always
Created attachment 85454 [details] implementation of the 2 ioctls with debug traces
Further investigation show that strace sees the same thing: open("/data", O_RDONLY|O_NONBLOCK|O_DIRECTORY) = 3 ioctl(3, 0xffffffffc0086696, 0x20cd040) = 0 So it doesn't appear to be a valgrind bug. But it may need a patch to mask the hi 32 bits of the opcode.
If you want you can prepare a new version of this patch without the debug statements such that the patch becomes suitable for inclusion in the official Valgrind source code tree.
Created attachment 87652 [details] patch for Lustre's FID2PATH ioctl This is the support for Lustre's OBD_IOC_FID2PATH ioctl. Strace on the kernel I'm using (2.6.32, Centos 6.5) reports: # strace lfs fid2path <FILE> <FID> ... ioctl(3, 0xffffffffc0086696, 0x2252040) = 0 ... I don't see why the high bits of the command are set. So to work around, the patch checks against both VKI_OBD_IOC_FID2PATH and VKI_OBD_IOC_FID2PATH | 0xffffffff00000000ULL.
After having had a look at the attached patch and also at the Lustre source code my questions are as follows: * Apparently the data type of the second argument of the ioctl() system call inside the Linux kernel is "unsigned int". Since Linux uses the LP64 model this means that the size of the second argument is 32 bits on 32-bit and 64-bit systems. Is it possible that the value "0xffffffffc0086696" shown by strace means that strace incorrectly sign-extended the command argument ? * In the Lustre source code I haven't found any code that modifies the gf_pathlen struct member. Does that mean that "POST_FIELD_WRITE(args->gf_pathlen)" should be removed ? * Which members of struct getinfo_fid2path are read by the FS_IOC_FIEMAP ioctl ? Is perhaps a pre-ioctl wrapper is missing ? Does the (untested) patch below make sense to you ? [PATCH] core: Add support for the Lustre ioctl OBD_IOC_FID2PATH (#331829) diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c index f83b59f..f058ed5 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c @@ -7067,6 +7067,12 @@ PRE(sys_ioctl) } #endif + case VKI_OBD_IOC_FID2PATH: { + struct vki_getinfo_fid2path *args = (void *)(ARG3); + PRE_MEM_READ("VKI_OBD_IOC_FID2PATH(args)", (Addr)args, sizeof(*args)); + } + break; + default: /* EVIOC* are variable length and return size written on success */ switch (ARG2 & ~(_VKI_IOC_SIZEMASK << _VKI_IOC_SIZESHIFT)) { @@ -8354,6 +8360,12 @@ POST(sys_ioctl) break; #endif + case VKI_OBD_IOC_FID2PATH: { + struct vki_getinfo_fid2path *args = (void *)(ARG3); + POST_MEM_WRITE((Addr)args->gf_path, args->gf_pathlen); + } + break; + default: /* EVIOC* are variable length and return size written on success */ switch (ARG2 & ~(_VKI_IOC_SIZEMASK << _VKI_IOC_SIZESHIFT)) { diff --git a/include/vki/vki-linux.h b/include/vki/vki-linux.h index 6043842..25b5db8 100644 --- a/include/vki/vki-linux.h +++ b/include/vki/vki-linux.h @@ -3420,6 +3420,40 @@ struct vki_ethtool_ts_info { #define VKI_ETHTOOL_SCHANNELS 0x0000003d /* Set no of channels */ #define VKI_ETHTOOL_GET_TS_INFO 0x00000041 /* Get time stamping and PHC info */ +//---------------------------------------------------------------------- +// From drivers/staging/lustre/lustre/include/lustre/lustre_user.h +//---------------------------------------------------------------------- + +struct vki_lu_fid { + __vki_u64 f_seq; + __vki_u32 f_oid; + __vki_u32 f_ver; +}; + +//---------------------------------------------------------------------- +// From drivers/staging/lustre/lustre/include/lustre/lustre_idl.h +//---------------------------------------------------------------------- + +struct vki_getinfo_fid2path { + struct vki_lu_fid gf_fid; + __vki_u64 gf_recno; + __vki_u32 gf_linkno; + __vki_u32 gf_pathlen; + char gf_path[0]; +} __attribute__((packed)); + +//---------------------------------------------------------------------- +// From drivers/staging/lustre/lustre/include/linux/lustre_lib.h +//---------------------------------------------------------------------- + +#define OBD_IOC_DATA_TYPE long + +//---------------------------------------------------------------------- +// From drivers/staging/lustre/lustre/include/lustre_lib.h +//---------------------------------------------------------------------- + +#define VKI_OBD_IOC_FID2PATH _VKI_IOWR ('f', 150, OBD_IOC_DATA_TYPE) + #endif // __VKI_LINUX_H /*--------------------------------------------------------------------*/
Hi Bart, I don't understand why there is a sign expansion. I suspect a bug somewhere in the libc or the kernel. For instance with this program: #include <sys/types.h> #include <fcntl.h> #include <sys/ioctl.h> int fd; void foo(int cmd) { ioctl(fd, cmd); } int main(void) { fd=open("/tmp", O_RDONLY); ioctl(fd, 0xc2345678); foo(0xc2345678); ioctl(fd, 0x1a34567812345678); return 0; } strace shows: ioctl(3, 0xc2345678, 0x7fff8f780428) = -1 ENOTTY (Inappropriate ioctl for device) ioctl(3, 0xffffffffc2345678, 0xffffffffc2345678) = -1 ENOTTY (Inappropriate ioctl for device) ioctl(3, 0x1a34567812345678, 0xffffffffc2345678) = -1 ENOTTY (Inappropriate ioctl for device) Valgrind shows: ==4644== Warning: noted but unhandled ioctl 0x1a34567812345678 with no size/direction hints So it seems that although the ioctl prototype says it's an int, it's really a long. Lustre is using an internal wrapper to call the ioctl for the fid2path ioctl, so it's hitting the second case. I haven't tried your patch yet, but I looks good (besides not working because of the sign extension). You're correct that "POST_FIELD_WRITE(args->gf_pathlen)" is not valid. Also I remove the patch for FS_IOC_FIEMAP because I haven't been able to test it recently. I will resubmit it someday.
Created attachment 87871 [details] Reworked Lustre FID2PATH ioctl patch Frank, can you either provide a patch in which the above comments have been addressed or test the attached patch called "Reworked Lustre FID2PATH ioctl patch" ?
Hi Bart, So this last patch works fine. Regarding the sign expansion, it comes from gcc's headers (/usr/include/x86_64-linux-gnu/sys/ioctl.h): extern int ioctl (int __fd, unsigned long int __request, ...) __THROW; So it works fine if the request is a constant. However when the ioctl call is wrapped in a function call (as shown in comment 6), then we go from an int to an unsigned long --> we get a sign extension. At that point I can't think lustre is the only affected software. I've already seen that kind of construction elsewhere. May be valgrind should mask the high bits of the opcode.
See also r14232 and r14233 on the trunk.