Thanks for reviewing Al...
On 26 March 2013 19:18, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Mar 26, 2013 at 03:19:29PM +0000, James Hogan wrote:
>> +struct da_finddata {
>> + unsigned long size;
>> + unsigned long attrib;
>> + char name[260];
>> +};
>
> ... especially for this - unlike int and short, long really is arch-dependent.
> And yes, I realize that this thing is arch-specific, but...
Yes, agreed.
>
>> +struct dafs_inode_info {
>> + int fd;
>> + int mode;
>
> umm... int or fmode_t?
Indeed. The same update to hostfs during v2.6.28 was missed :-(
>
>> + struct inode vfs_inode;
>> +};
>
>> +#define DAFS_SUPER_MAGIC 0xdadadaf5
>
> -> linux/magic.h
Okay. hostfs has the same problem.
>
>> +static char *dentry_name(struct dentry *dentry)
>> +{
>> + char *name = __getname();
>> + if (!name)
>> + return NULL;
>> +
>> + return __dentry_name(dentry, name); /* will unlock */
>
> will unlock what?
The change adding this was based on hostfs, which had the comment
added by you in e9193059b1b3733695d5b80e667778311695aa73 ("hostfs: fix
races in dentry_name() and inode_name()"), along with a spin_lock just
before it. The spin_lock (and unlock in __dentry_name) was
subsequently removed in ec2447c278ee973d35f38e53ca16ba7f965ae33d
("hostfs: simplify locking"), so the comment is no longer relevant.
I'll remove it.
>
>> + hi = kzalloc(sizeof(*hi), GFP_KERNEL);
>> + if (hi == NULL)
>> + return NULL;
>> +
>> + hi->fd = -1;
>> + inode_init_once(&hi->vfs_inode);
>> + return &hi->vfs_inode;
>
> Umm... kzalloc() looks odd, seeing that you proceed to initialize two
> fields out of 3 (including the really big one) explicitly...
Agreed, another issue in common with hostfs. I'll fix.
>
>> +static int open_dir(char *path, struct da_finddata *finddata, int *fserrno)
>> +{
>> + int len = strlen(path);
>> + char buf[len + 3];
>
> Bad idea, considering that strlen(path) is user-controlled and can
> theoretically go up to 4Kb...
Ouch yes, I'll convert it to use kmalloc/kfree (the fs operations are
horribly slow anyway so it's never a performance issue).
>
>> +static int dafs_readdir(struct file *file, void *ent, filldir_t filldir)
>> +{
>> + struct inode *inode = file->f_path.dentry->d_inode;
>
> minor nit, but... that's file_inode(file)
Ooh yes.
>
>> + name = dentry_name(file->f_path.dentry);
>> + if (name == NULL)
>> + return -ENOMEM;
>> + handle = open_dir(name, &finddata, &fserrno);
>
> ... and what if the sucker's parent gets renamed between dentry_name() and
> open_dir()? The same goes for the rest of dentry_name() users, actually.
It would seem impossible to protect against this happening from the
file server side, and as for the Linux side...
>
>> +static int dafs_rename(struct inode *from_ino, struct dentry *from,
>> + struct inode *to_ino, struct dentry *to)
>> +{
>> + char *from_name, *to_name;
>> + int err;
>> +
>> + from_name = dentry_name(from);
>> + if (from_name == NULL)
>> + return -ENOMEM;
>> + to_name = dentry_name(to);
>> + if (to_name == NULL) {
>> + __putname(from_name);
>> + return -ENOMEM;
>> + }
>> + err = -EINVAL;
>> + __putname(from_name);
>> + __putname(to_name);
>> + return err;
>> +}
>
> Aha. No rename(2), IOW. Is one planned?
Hmm, indeed it appears a rename DAFS operation hasn't been defined.
I'll double check that tomorrow. DAFS is usually used for just copying
data to/from a target (e.g. an fpga with no other connectivity) so the
lack of rename isn't usually a problem.
I'll fix up these issues (and do some separate patches for hostfs too).
Thanks
James
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
