| Message ID | YgmB01p+p45Cihhg@p100 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fat: Use pointer to d_name[0] in put_user() for compat case | expand |
Helge Deller <deller@gmx.de> writes: > The put_user(val,ptr) macro wants a pointer in the second parameter, but in > fat_ioctl_filldir() the d_name field references a whole "array of chars". > Usually the compiler automatically converts it and uses a pointer to that > array, but it's more clean to explicitly give the real pointer to where someting > is put, which is in this case the first character of the d_name[] array. > > I noticed that issue while trying to optimize the parisc put_user() macro > and used an intermediate variable to store the pointer. In that case I > got this error: > > In file included from include/linux/uaccess.h:11, > from include/linux/compat.h:17, > from fs/fat/dir.c:18: > fs/fat/dir.c: In function ‘fat_ioctl_filldir’: > fs/fat/dir.c:725:33: error: invalid initializer > 725 | if (put_user(0, d2->d_name) || \ > | ^~ > include/asm/uaccess.h:152:33: note: in definition of macro ‘__put_user’ > 152 | __typeof__(ptr) __ptr = ptr; \ > | ^~~ > fs/fat/dir.c:759:1: note: in expansion of macro ‘FAT_IOCTL_FILLDIR_FUNC’ > 759 | FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent) > > The patch below cleans it up. > > Signed-off-by: Helge Deller <deller@gmx.de> Looks good. Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> > diff --git a/fs/fat/dir.c b/fs/fat/dir.c > index c4a274285858..249825017da7 100644 > --- a/fs/fat/dir.c > +++ b/fs/fat/dir.c > @@ -722,7 +722,7 @@ static int func(struct dir_context *ctx, const char *name, int name_len, \ > if (name_len >= sizeof(d1->d_name)) \ > name_len = sizeof(d1->d_name) - 1; \ > \ > - if (put_user(0, d2->d_name) || \ > + if (put_user(0, &d2->d_name[0]) || \ > put_user(0, &d2->d_reclen) || \ > copy_to_user(d1->d_name, name, name_len) || \ > put_user(0, d1->d_name + name_len) || \
From: Helge Deller > Sent: 13 February 2022 22:10 > > The put_user(val,ptr) macro wants a pointer in the second parameter, but in > fat_ioctl_filldir() the d_name field references a whole "array of chars". > Usually the compiler automatically converts it and uses a pointer to that > array, but it's more clean to explicitly give the real pointer to where someting > is put, which is in this case the first character of the d_name[] array. That just isn't true. In C both x->char_array and &x->char_array[0] have the same type 'char *'. The 'bug' is caused by put_user() trying to do: __typeof__(ptr) __ptr = ptr; where __typeof__ is returning char[n] not char *. I've tried a few things but can't get __typeof__ to generate a suitable type for both a simple type and array. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 14 February 2022 09:12 > > From: Helge Deller > > Sent: 13 February 2022 22:10 > > > > The put_user(val,ptr) macro wants a pointer in the second parameter, but in > > fat_ioctl_filldir() the d_name field references a whole "array of chars". > > Usually the compiler automatically converts it and uses a pointer to that > > array, but it's more clean to explicitly give the real pointer to where someting > > is put, which is in this case the first character of the d_name[] array. > > That just isn't true. > > In C both x->char_array and &x->char_array[0] have the same type > 'char *'. > > The 'bug' is caused by put_user() trying to do: > __typeof__(ptr) __ptr = ptr; > where __typeof__ is returning char[n] not char *. > > I've tried a few things but can't get __typeof__ to > generate a suitable type for both a simple type and array. Actually the issue is that put_user() writes a single variable and needs a pointer to one. So changing to: put_user(0, &array[0]); is probably fine. But the description is all wrong. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Feb 14 2022, David Laight wrote: > The 'bug' is caused by put_user() trying to do: > __typeof__(ptr) __ptr = ptr; > where __typeof__ is returning char[n] not char *. > > I've tried a few things but can't get __typeof__ to > generate a suitable type for both a simple type and array. Does it work to use __typeof__(&*(ptr))?
On 2/14/22 10:27, Andreas Schwab wrote: > On Feb 14 2022, David Laight wrote: > >> The 'bug' is caused by put_user() trying to do: >> __typeof__(ptr) __ptr = ptr; >> where __typeof__ is returning char[n] not char *. >> >> I've tried a few things but can't get __typeof__ to >> generate a suitable type for both a simple type and array. > > Does it work to use __typeof__(&*(ptr))? Yes, this works. Helge
On 2/14/22 10:26, David Laight wrote: > From: David Laight >> Sent: 14 February 2022 09:12 >> >> From: Helge Deller >>> Sent: 13 February 2022 22:10 >>> >>> The put_user(val,ptr) macro wants a pointer in the second parameter, but in >>> fat_ioctl_filldir() the d_name field references a whole "array of chars". >>> Usually the compiler automatically converts it and uses a pointer to that >>> array, but it's more clean to explicitly give the real pointer to where someting >>> is put, which is in this case the first character of the d_name[] array. >> >> That just isn't true. >> >> In C both x->char_array and &x->char_array[0] have the same type >> 'char *'. >> >> The 'bug' is caused by put_user() trying to do: >> __typeof__(ptr) __ptr = ptr; >> where __typeof__ is returning char[n] not char *. >> >> I've tried a few things but can't get __typeof__ to >> generate a suitable type for both a simple type and array. > > Actually the issue is that put_user() writes a single variable > and needs a pointer to one. > So changing to: > put_user(0, &array[0]); > is probably fine. Ok. > But the description is all wrong. I agree it can be improved. Would you mind proposing a better description? Helge
From: Helge Deller > Sent: 14 February 2022 11:05 > > On 2/14/22 10:26, David Laight wrote: > > From: David Laight > >> Sent: 14 February 2022 09:12 > >> > >> From: Helge Deller > >>> Sent: 13 February 2022 22:10 > >>> > >>> The put_user(val,ptr) macro wants a pointer in the second parameter, but in > >>> fat_ioctl_filldir() the d_name field references a whole "array of chars". > >>> Usually the compiler automatically converts it and uses a pointer to that > >>> array, but it's more clean to explicitly give the real pointer to where someting > >>> is put, which is in this case the first character of the d_name[] array. > >> > >> That just isn't true. > >> > >> In C both x->char_array and &x->char_array[0] have the same type > >> 'char *'. > >> > >> The 'bug' is caused by put_user() trying to do: > >> __typeof__(ptr) __ptr = ptr; > >> where __typeof__ is returning char[n] not char *. > >> > >> I've tried a few things but can't get __typeof__ to > >> generate a suitable type for both a simple type and array. > > > > Actually the issue is that put_user() writes a single variable > > and needs a pointer to one. > > So changing to: > > put_user(0, &array[0]); > > is probably fine. > > Ok. > > > But the description is all wrong. > > I agree it can be improved. > Would you mind proposing a better description? put_user() needs a pointer to a simple type. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/fs/fat/dir.c b/fs/fat/dir.c index c4a274285858..249825017da7 100644 --- a/fs/fat/dir.c +++ b/fs/fat/dir.c @@ -722,7 +722,7 @@ static int func(struct dir_context *ctx, const char *name, int name_len, \ if (name_len >= sizeof(d1->d_name)) \ name_len = sizeof(d1->d_name) - 1; \ \ - if (put_user(0, d2->d_name) || \ + if (put_user(0, &d2->d_name[0]) || \ put_user(0, &d2->d_reclen) || \ copy_to_user(d1->d_name, name, name_len) || \ put_user(0, d1->d_name + name_len) || \
The put_user(val,ptr) macro wants a pointer in the second parameter, but in fat_ioctl_filldir() the d_name field references a whole "array of chars". Usually the compiler automatically converts it and uses a pointer to that array, but it's more clean to explicitly give the real pointer to where someting is put, which is in this case the first character of the d_name[] array. I noticed that issue while trying to optimize the parisc put_user() macro and used an intermediate variable to store the pointer. In that case I got this error: In file included from include/linux/uaccess.h:11, from include/linux/compat.h:17, from fs/fat/dir.c:18: fs/fat/dir.c: In function ‘fat_ioctl_filldir’: fs/fat/dir.c:725:33: error: invalid initializer 725 | if (put_user(0, d2->d_name) || \ | ^~ include/asm/uaccess.h:152:33: note: in definition of macro ‘__put_user’ 152 | __typeof__(ptr) __ptr = ptr; \ | ^~~ fs/fat/dir.c:759:1: note: in expansion of macro ‘FAT_IOCTL_FILLDIR_FUNC’ 759 | FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent) The patch below cleans it up. Signed-off-by: Helge Deller <deller@gmx.de>