[linux-cifs-client] [PATCH 6/7] [CIFS] when not using unix extensions, check for and set ATTR_READONLY on create

[Guenter Kukkukk]

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.

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,