Re: [linux-cifs-client[[patch] Attempt #2 to handle null nameidata

On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <[email protected]> wrote:
> On Wed,  7 Apr 2010 11:19:10 -0500
> [email protected] wrote:
>
>> While creating a file on a server which supports unix extensions
>> such as Samba, if a file is being created which does not supply
>> nameidata (i.e. nd is null), cifs client can oops when calling
>> cifs_posix_open.
>>
>> Signed-off-by: Shirish Pargaonkar <[email protected]>
>> Reported-by: Eugene Teo <[email protected]>
>> ---
>>
>
> This patch is hideous, but I suppose it's the best that can be done
> short of rewriting this code to have a sane API.
>
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index 88e2bc4..efb8772 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -95,8 +95,10 @@ extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
>>                               __u16 fileHandle, struct file *file,
>>                               struct vfsmount *mnt, unsigned int oflags);
>>  extern int cifs_posix_open(char *full_path, struct inode **pinode,
>> -                        struct vfsmount *mnt, int mode, int oflags,
>> -                        __u32 *poplock, __u16 *pnetfid, int xid);
>> +                             struct vfsmount *mnt,
>> +                             struct super_block *sb,
>> +                             int mode, int oflags,
>> +                             __u32 *poplock, __u16 *pnetfid, int xid);
>>  extern void cifs_unix_basic_to_fattr(struct cifs_fattr *fattr,
>>                                    FILE_UNIX_BASIC_INFO *info,
>>                                    struct cifs_sb_info *cifs_sb);
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index 6ccf726..9e9d48f 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -183,13 +183,14 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
>>  }
>>
>>  int cifs_posix_open(char *full_path, struct inode **pinode,
>> -                 struct vfsmount *mnt, int mode, int oflags,
>> -                 __u32 *poplock, __u16 *pnetfid, int xid)
>> +                     struct vfsmount *mnt, struct super_block *sb,
>> +                     int mode, int oflags,
>> +                     __u32 *poplock, __u16 *pnetfid, int xid)
>>  {
>>       int rc;
>>       FILE_UNIX_BASIC_INFO *presp_data;
>>       __u32 posix_flags = 0;
>> -     struct cifs_sb_info *cifs_sb = CIFS_SB(mnt->mnt_sb);
>> +     struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>>       struct cifs_fattr fattr;
>>
>>       cFYI(1, ("posix open %s", full_path));
>> @@ -242,7 +243,7 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>>
>>       /* get new inode and set it up */
>>       if (*pinode == NULL) {
>> -             *pinode = cifs_iget(mnt->mnt_sb, &fattr);
>> +             *pinode = cifs_iget(sb, &fattr);
>>               if (!*pinode) {
>>                       rc = -ENOMEM;
>>                       goto posix_open_ret;
>> @@ -251,7 +252,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>>               cifs_fattr_to_inode(*pinode, &fattr);
>>       }
>>
>> -     cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
>> +     if (mnt)
>> +             cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
>>
>
> The cifs_create codepath closes the file (via CIFSSMBClose) when nd is
> NULL. The NULL mnt case here is analogous, right? Shouldn't it also
> CIFSSMBClose the file?

Jeff, not sure I understand.  In case of null nd, cifs_create will
close the file
whether posix open or regular open (or legacy open) gets called or not.

Regards,

Shirish

>
>>  posix_open_ret:
>>       kfree(presp_data);
>> @@ -315,13 +317,14 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>>       if (nd && (nd->flags & LOOKUP_OPEN))
>>               oflags = nd->intent.open.flags;
>>       else
>> -             oflags = FMODE_READ;
>> +             oflags = FMODE_READ | SMB_O_CREAT;
>>
>>       if (tcon->unix_ext && (tcon->ses->capabilities & CAP_UNIX) &&
>>           (CIFS_UNIX_POSIX_PATH_OPS_CAP &
>>                       le64_to_cpu(tcon->fsUnixInfo.Capability))) {
>> -             rc = cifs_posix_open(full_path, &newinode, nd->path.mnt,
>> -                                  mode, oflags, &oplock, &fileHandle, xid);
>> +             rc = cifs_posix_open(full_path, &newinode,
>> +                     nd ? nd->path.mnt : NULL,
>> +                     inode->i_sb, mode, oflags, &oplock, &fileHandle, xid);
>>               /* EIO could indicate that (posix open) operation is not
>>                  supported, despite what server claimed in capability
>>                  negotation.  EREMOTE indicates DFS junction, which is not
>> @@ -678,6 +681,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>>                    (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
>>                    (nd->intent.open.flags & O_CREAT)) {
>>                       rc = cifs_posix_open(full_path, &newInode, nd->path.mnt,
>> +                                     parent_dir_inode->i_sb,
>>                                       nd->intent.open.create_mode,
>>                                       nd->intent.open.flags, &oplock,
>>                                       &fileHandle, xid);
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index 3d8f8a9..503a459 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -297,10 +297,12 @@ int cifs_open(struct inode *inode, struct file *file)
>>           (CIFS_UNIX_POSIX_PATH_OPS_CAP &
>>                       le64_to_cpu(tcon->fsUnixInfo.Capability))) {
>>               int oflags = (int) cifs_posix_convert_flags(file->f_flags);
>> +             oflags |= SMB_O_CREAT;
>>               /* can not refresh inode info since size could be stale */
>>               rc = cifs_posix_open(full_path, &inode, file->f_path.mnt,
>> -                                  cifs_sb->mnt_file_mode /* ignored */,
>> -                                  oflags, &oplock, &netfid, xid);
>> +                             inode->i_sb,
>> +                             cifs_sb->mnt_file_mode /* ignored */,
>> +                             oflags, &oplock, &netfid, xid);
>>               if (rc == 0) {
>>                       cFYI(1, ("posix open succeeded"));
>>                       /* no need for special case handling of setting mode
>> @@ -512,8 +514,9 @@ reopen_error_exit:
>>               int oflags = (int) cifs_posix_convert_flags(file->f_flags);
>>               /* can not refresh inode info since size could be stale */
>>               rc = cifs_posix_open(full_path, NULL, file->f_path.mnt,
>> -                                  cifs_sb->mnt_file_mode /* ignored */,
>> -                                  oflags, &oplock, &netfid, xid);
>> +                             inode->i_sb,
>> +                             cifs_sb->mnt_file_mode /* ignored */,
>> +                             oflags, &oplock, &netfid, xid);
>>               if (rc == 0) {
>>                       cFYI(1, ("posix reopen succeeded"));
>>                       goto reopen_success;
>> _______________________________________________
>> linux-cifs-client mailing list
>> [email protected]
>> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>
>
> Ugly, but I suppose it's all that can reasonably be done short of
> making all of this code use a more sane API.
>
> --
> Jeff Layton <[email protected]>
>