[linux-cifs-client] [PATCH 6/7] [CIFS] when not using unix
extensions, check for and set ATTR_READONLY on create
Guenter Kukkukk
linux at kukkukk.com
Tue May 6 23:35:46 GMT 2008
Am Dienstag, 6. Mai 2008 schrieb Jeff Layton:
> On Tue, 6 May 2008 04:48:42 +0200
> Guenter Kukkukk <linux at kukkukk.com> wrote:
>
> > Am Montag, 5. Mai 2008 schrieb Jeff Layton:
> > > When creating a file, set ATTR_READONLY if the create mode has no write
> > > bits set and we're not using unix extensions.
> > >
> > > There are some comments about this being problematic due to the VFS
> > > splitting creates into 2 parts. I'm not sure what that's actually
> > > talking about, but I'm assuming that it has something to do with how
> > > mknod is implemented. In the simple case where we have no unix
> > > extensions and we're just creating a regular file, there's no reason
> > > we can't set ATTR_READONLY.
> > >
> > > Signed-off-by: Jeff Layton <jlayton at redhat.com>
> > > ---
> > > fs/cifs/cifspdu.h | 1 +
> > > fs/cifs/cifssmb.c | 16 ++++++----------
> > > fs/cifs/dir.c | 16 +++++++++++++---
> > > 3 files changed, 20 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
> > > index a0d26b5..c43bf4b 100644
> > > --- a/fs/cifs/cifspdu.h
> > > +++ b/fs/cifs/cifspdu.h
> > > @@ -340,6 +340,7 @@
> > > #define OPEN_NO_RECALL 0x00400000
> > > #define OPEN_FREE_SPACE_QUERY 0x00800000 /* should be zero */
> > > #define CREATE_OPTIONS_MASK 0x007FFFFF
> > > +#define CREATE_OPTION_READONLY 0x10000000
> > > #define CREATE_OPTION_SPECIAL 0x20000000 /* system. NB not sent over wire */
> > >
> > > /* ImpersonationLevel flags */
> > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > > index cfd9750..95fbba4 100644
> > > --- a/fs/cifs/cifssmb.c
> > > +++ b/fs/cifs/cifssmb.c
> > > @@ -1224,11 +1224,8 @@ OldOpenRetry:
> > > else /* BB FIXME BB */
> > > pSMB->FileAttributes = cpu_to_le16(0/*ATTR_NORMAL*/);
> > >
> > > - /* if ((omode & S_IWUGO) == 0)
> > > - pSMB->FileAttributes |= cpu_to_le32(ATTR_READONLY);*/
> > > - /* Above line causes problems due to vfs splitting create into two
> > > - pieces - need to set mode after file created not while it is
> > > - being created */
> > > + if (create_options & CREATE_OPTION_READONLY)
> > > + pSMB->FileAttributes |= cpu_to_le16(ATTR_READONLY);
> > >
> >
> > Hi Jeff, Steve,
> >
> > ouch, the "legacy observer is creeping in here". :-)
> > some lines above the code reads:
> > ...
> > /* BB fixme add conversion for access_flags to bits 0 - 2 of mode */
> > /* 0 = read
> > 1 = write
> > 2 = rw
> > 3 = execute
> > */
> > pSMB->Mode = cpu_to_le16(2); !!!!!!!! ALWAYS RW
> > pSMB->Mode |= cpu_to_le16(0x40); /* deny none */
> > ...
> >
> > at least the passed "ntcreateandx" like "access_flags" must be
> > parsed here to match legacy semantics.
> > The current "always RW" approach leads to a simple copy failure
> > cp src dst
> > when src is set readonly - and the open() requests RW!
> > Care must be taken here to convert "access_flags", i.e.
> > FILE_WRITE_ATTRIBUTES is also been used to request write
> > access.
> > So we need a "matching conversion" function here.
> >
>
> I had to look over your comments a couple of times. I'm assuming that
> you're not seeing a problem with the patch I've proposed, but rather
> pointing out a problem with some existing code that's already there.
>
> If that is the case, then proposing a patch independent of this one
> might be less confusing. If that's not the case, then could you point
> out what you think this particular patch is doing wrong?
Sorry for having been a bit unclear here. :-)
I agree with your patch.
In addition a new patch should be written to fix the missing "access_flags"
conversion.
Cheers, Günter
>
> > > /* BB FIXME BB */
> > > /* pSMB->CreateOptions = cpu_to_le32(create_options &
> > > @@ -1331,17 +1328,16 @@ openRetry:
> > > pSMB->FileAttributes = cpu_to_le32(ATTR_SYSTEM);
> > > else
> > > pSMB->FileAttributes = cpu_to_le32(ATTR_NORMAL);
> > > +
> > > /* XP does not handle ATTR_POSIX_SEMANTICS */
> > > /* but it helps speed up case sensitive checks for other
> > > servers such as Samba */
> > > if (tcon->ses->capabilities & CAP_UNIX)
> > > pSMB->FileAttributes |= cpu_to_le32(ATTR_POSIX_SEMANTICS);
> > >
> > > - /* if ((omode & S_IWUGO) == 0)
> > > - pSMB->FileAttributes |= cpu_to_le32(ATTR_READONLY);*/
> > > - /* Above line causes problems due to vfs splitting create into two
> > > - pieces - need to set mode after file created not while it is
> > > - being created */
> > > + if (create_options & CREATE_OPTION_READONLY)
> > > + pSMB->FileAttributes |= cpu_to_le32(ATTR_READONLY);
> > > +
> > > pSMB->ShareAccess = cpu_to_le32(FILE_SHARE_ALL);
> > > pSMB->CreateDisposition = cpu_to_le32(openDisposition);
> > > pSMB->CreateOptions = cpu_to_le32(create_options & CREATE_OPTIONS_MASK);
> > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > > index 84abd0a..c159734 100644
> > > --- a/fs/cifs/dir.c
> > > +++ b/fs/cifs/dir.c
> > > @@ -119,6 +119,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> > > {
> > > int rc = -ENOENT;
> > > int xid;
> > > + int create_options = CREATE_NOT_DIR;
> > > int oplock = 0;
> > > int desiredAccess = GENERIC_READ | GENERIC_WRITE;
> > > __u16 fileHandle;
> > > @@ -176,9 +177,19 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> > > FreeXid(xid);
> > > return -ENOMEM;
> > > }
> > > +
> > > + mode &= ~current->fs->umask;
> > > +
> > > + /*
> > > + * if we're not using unix extensions, see if we need to set
> > > + * ATTR_READONLY on the create call
> > > + */
> > > + if (!pTcon->unix_ext && (mode & S_IWUGO) == 0)
> > > + create_options |= CREATE_OPTION_READONLY;
> > > +
> > > if (cifs_sb->tcon->ses->capabilities & CAP_NT_SMBS)
> > > rc = CIFSSMBOpen(xid, pTcon, full_path, disposition,
> > > - desiredAccess, CREATE_NOT_DIR,
> > > + desiredAccess, create_options,
> > > &fileHandle, &oplock, buf, cifs_sb->local_nls,
> > > cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> > > else
> > > @@ -187,7 +198,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> > > if (rc == -EIO) {
> > > /* old server, retry the open legacy style */
> > > rc = SMBLegacyOpen(xid, pTcon, full_path, disposition,
> > > - desiredAccess, CREATE_NOT_DIR,
> > > + desiredAccess, create_options,
> > > &fileHandle, &oplock, buf, cifs_sb->local_nls,
> > > cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> > > }
> > > @@ -197,7 +208,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> > > /* If Open reported that we actually created a file
> > > then we now have to set the mode if possible */
> > > if ((pTcon->unix_ext) && (oplock & CIFS_CREATE_ACTION)) {
> > > - mode &= ~current->fs->umask;
> > > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
> > > CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode,
> > > (__u64)current->fsuid,
> >
> >
>
More information about the linux-cifs-client
mailing list