Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH 17/25] vfat: move ioctl32 code to fs/fat/dir.c | Date | Mon, 7 Nov 2005 11:41:33 +0100 |
| |
On Sünndag 06 November 2005 13:05, OGAWA Hirofumi wrote: > Arnd Bergmann <arnd@arndb.de> writes: > > > +#ifdef CONFIG_COMPAT > > +/* vfat */ > > +#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2]) > > +#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2]) > ^^^^^^ > > Please use a SPACE after #define, and kill a useless "/* vfat */".
ok.
> > +static long > > +put_dirent32(struct dirent *d, struct compat_dirent __user * d32) > > +{ > > + if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent))) > > + return -EFAULT; > > + > > + __put_user(d->d_ino, &d32->d_ino); > > + __put_user(d->d_off, &d32->d_off); > > + __put_user(d->d_reclen, &d32->d_reclen); > > + if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen)) > > + return -EFAULT; > > Why don't we need to check the return value of __put_user()?
This is a bug in the code that I copied over, and it needs to be fixed, thanks.
> > + set_fs(KERNEL_DS); > > + ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d); > > + set_fs(oldfs); > > + if (ret >= 0) { > > + ret |= put_dirent32(&d[0], p); > > + ret |= put_dirent32(&d[1], p + 1); > > If "ret" is not 0, we can't use "|=" here? > > This patch seems to have bugs, although original one too.
Right. In my patches I tried to change as little as possible to avoid introducing new bugs, but this one is already so broken, that it's better to do a clean implementation from scratch.
Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |