[PATCH linux-next] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size()

Jeff Layton jlayton at redhat.com
Wed Nov 27 04:27:57 MST 2013


On Tue, 26 Nov 2013 17:12:38 -0700
Tim Gardner <timg at tpi.com> wrote:

> According to Microsoft documentation:
> 		http://msdn.microsoft.com/en-us/library/ee441943.aspx
> 		http://msdn.microsoft.com/en-us/library/ff469854.aspx
> 
> The information level codes used in CIFSSMBSetEOF() are not
> legal. In practice we have found that they really don't work either.
> The specification clearly states that none of the SMB_SET_FILE infornmation
> level codes are supported for TRANS2_SET_PATH_INFORMATION.
> 

Well spotted!

> You can trigger this function with the following C code. Note that O_CREAT|O_TRUNC
> is not a valid combination, but it does serve to illustrate the bug.
> 

FWIW, O_CREAT|O_TRUNC is a valid combination. What's not valid in the
program below is the fact that you don't set O_RDWR or O_WRONLY, which
may give you problems on some filesystems.


> cat <<EOF > truncate.c
> 
> int main(int argc, char *argv[])
> {
> 	char *fname;
> 	int fd;
> 	mode_t mode = S_IRUSR|S_IWUSR;
> 	int flags = O_CREAT|O_TRUNC;
> 
> 	if (argc == 2) {
> 		fname = argv[1];
> 	} else {
> 		printf("You must supply a file name.\n");
> 		exit(1);
> 	}
> 
> 	if ((fd=open(fname,flags,mode))  < 0)
> 	{
> 		printf("could not open %s with flags %08x\n",fname,flags);
> 		exit(1);
> 	}
> 	printf("Opened %s with flags %08x\n",fname,flags);
> 
> 	close(fd);
> }
> EOF
> 
> I used this script:
> 
> cat <<EOF > truncate.sh
> LOG=log.txt
> SRV=10.0.0.184
> MP=/tmp/mnt
> TD=$MP/temp
> 
> gcc -o truncate truncate.c
> rm -f $LOG
> 
> sudo mkdir -p $MP
> sudo mount -t cifs //$SRV/test $MP --verbose -o noserverino,nounix,user=test,pass=test
> 
> mkdir -p $TD
> sudo rm -f $TD/junk.txt
> echo 1 > junk.txt
> sudo cp junk.txt $TD/junk.txt
> sudo dmesg -c > /dev/null
> 
> echo 1 | sudo tee /proc/fs/cifs/cifsFYI
> sudo ./truncate $TD/junk.txt
> echo 0 | sudo tee /proc/fs/cifs/cifsFYI
> 
> sudo umount $MP
> sudo dmesg -c > $LOG
> EOF
> 
> The resulting log file:
> 
> [179777.458893] fs/cifs/file.c:cifs_open:447: CIFS VFS: in cifs_open as Xid: 654 with uid: 0
> [179777.458903] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
> [179777.458907] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
> [179777.458922] fs/cifs/file.c:cifs_open:465: inode = 0xffff88005a42b828 file flags are 0x8240 for \temp\junk.txt
> [179777.458935] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 162
> [179777.458938] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=114
> [179777.460089] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x67
> [179777.460109] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x6b, smb_buf_length: 0x67
> [179777.460125] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=162 mid=20 state=4
> [179777.460134] fs/cifs/inode.c:cifs_get_inode_info:646: Getting info on \temp\junk.txt
> [179777.460138] fs/cifs/inode.c:cifs_revalidate_cache:95: cifs_revalidate_cache: revalidating inode 320
> [179777.460141] fs/cifs/inode.c:cifs_revalidate_cache:119: cifs_revalidate_cache: invalidating inode 320 mapping
> [179777.460148] fs/cifs/file.c:cifs_open:545: CIFS VFS: leaving cifs_open (xid = 654) rc = 0
> [179777.460153] fs/cifs/inode.c:cifs_setattr:2280: name junk.txt size 0
> [179777.460157] fs/cifs/inode.c:cifs_setattr_nounix:2126: CIFS VFS: in cifs_setattr_nounix as Xid: 655 with uid: 0
> [179777.460160] fs/cifs/inode.c:cifs_setattr_nounix:2129: setattr on file junk.txt attrs->iavalid 0xa068
> [179777.460163] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
> [179777.460166] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
> [179777.460170] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5403: In SetEOF
> [179777.460175] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 50
> [179777.460178] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=112
> [179777.460810] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
> [179777.460827] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
> [179777.460830] fs/cifs/smb1ops.c:check2ndT2:255: invalid transact2 word count
> [179777.460841] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=50 mid=21 state=4
> [179777.460845] fs/cifs/netmisc.c:map_smb_to_linux_error:891: Mapping smb error code 0xc0000022 to POSIX err -13
> [179777.460849] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5469: SetPathInfo (file size) returned -13
> [179777.460851] fs/cifs/inode.c:cifs_set_file_size:1934: SetEOF by path (setattrs) rc = -13
> [179777.460855] fs/cifs/inode.c:cifs_setattr_nounix:2269: CIFS VFS: leaving cifs_setattr_nounix (xid = 655) rc = -13
> [179777.460863] fs/cifs/file.c:cifsFileInfo_put:385: closing last open instance for inode ffff88005a42b828
> [179777.460866] fs/cifs/file.c:cifsFileInfo_put:403: CIFS VFS: in cifsFileInfo_put as Xid: 656 with uid: 0
> [179777.460869] fs/cifs/cifssmb.c:CIFSSMBClose:2457: In CIFSSMBClose
> [179777.460873] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 4
> [179777.460876] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=41
> [179777.461257] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
> [179777.461267] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
> [179777.461280] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=4 mid=22 state=4
> 
> Cc: Jeff Layton <jlayton at redhat.com>
> Cc: Steve French <sfrench at samba.org>
> Signed-off-by: Dean Gehnert <deang at tpi.com>
> Signed-off-by: Tim Gardner <timg at tpi.com>
> ---
> 
> We encountered this problem whilst backporting the CIFS driver to a kernel and VFS old enough
> to still call inode_operations.setattr() when truncating a file. We're not sure it is the
> right upstream solution, but it worked for us on this older kernel.
> 
>  fs/cifs/cifsproto.h |  3 --
>  fs/cifs/cifssmb.c   | 98 -----------------------------------------------------
>  fs/cifs/smb1ops.c   | 36 +++++++++++++++++++-
>  3 files changed, 35 insertions(+), 102 deletions(-)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index aa33976..fe69b9c 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -296,9 +296,6 @@ extern int CIFSSMBSetAttrLegacy(unsigned int xid, struct cifs_tcon *tcon,
>  			char *fileName, __u16 dos_attributes,
>  			const struct nls_table *nls_codepage);
>  #endif /* possibly unneeded function */
> -extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> -			 const char *file_name, __u64 size,
> -			 struct cifs_sb_info *cifs_sb, bool set_allocation);
>  extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>  			      struct cifsFileInfo *cfile, __u64 size,
>  			      bool set_allocation);
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 124aa02..53f1b9b 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -5453,104 +5453,6 @@ QFSPosixRetry:
>  	return rc;
>  }
>  
> -
> -/*
> - * We can not use write of zero bytes trick to set file size due to need for
> - * large file support. Also note that this SetPathInfo is preferred to
> - * SetFileInfo based method in next routine which is only needed to work around
> - * a sharing violation bugin Samba which this routine can run into.
> - */
> -int
> -CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> -	      const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
> -	      bool set_allocation)
> -{
> -	struct smb_com_transaction2_spi_req *pSMB = NULL;
> -	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> -	struct file_end_of_file_info *parm_data;
> -	int name_len;
> -	int rc = 0;
> -	int bytes_returned = 0;
> -	int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
> -
> -	__u16 params, byte_count, data_count, param_offset, offset;
> -
> -	cifs_dbg(FYI, "In SetEOF\n");
> -SetEOFRetry:
> -	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
> -		      (void **) &pSMBr);
> -	if (rc)
> -		return rc;
> -
> -	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
> -		name_len =
> -		    cifsConvertToUTF16((__le16 *) pSMB->FileName, file_name,
> -				       PATH_MAX, cifs_sb->local_nls, remap);
> -		name_len++;	/* trailing null */
> -		name_len *= 2;
> -	} else {	/* BB improve the check for buffer overruns BB */
> -		name_len = strnlen(file_name, PATH_MAX);
> -		name_len++;	/* trailing null */
> -		strncpy(pSMB->FileName, file_name, name_len);
> -	}
> -	params = 6 + name_len;
> -	data_count = sizeof(struct file_end_of_file_info);
> -	pSMB->MaxParameterCount = cpu_to_le16(2);
> -	pSMB->MaxDataCount = cpu_to_le16(4100);
> -	pSMB->MaxSetupCount = 0;
> -	pSMB->Reserved = 0;
> -	pSMB->Flags = 0;
> -	pSMB->Timeout = 0;
> -	pSMB->Reserved2 = 0;
> -	param_offset = offsetof(struct smb_com_transaction2_spi_req,
> -				InformationLevel) - 4;
> -	offset = param_offset + params;
> -	if (set_allocation) {
> -		if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
> -			pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO2);
> -		else
> -			pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO);
> -	} else /* Set File Size */  {
> -	    if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
> -		    pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO2);
> -	    else
> -		    pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO);
> -	}
> -
> -	parm_data =
> -	    (struct file_end_of_file_info *) (((char *) &pSMB->hdr.Protocol) +
> -				       offset);
> -	pSMB->ParameterOffset = cpu_to_le16(param_offset);
> -	pSMB->DataOffset = cpu_to_le16(offset);
> -	pSMB->SetupCount = 1;
> -	pSMB->Reserved3 = 0;
> -	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
> -	byte_count = 3 /* pad */  + params + data_count;
> -	pSMB->DataCount = cpu_to_le16(data_count);
> -	pSMB->TotalDataCount = pSMB->DataCount;
> -	pSMB->ParameterCount = cpu_to_le16(params);
> -	pSMB->TotalParameterCount = pSMB->ParameterCount;
> -	pSMB->Reserved4 = 0;
> -	inc_rfc1001_len(pSMB, byte_count);
> -	parm_data->FileSize = cpu_to_le64(size);
> -	pSMB->ByteCount = cpu_to_le16(byte_count);
> -	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
> -			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
> -	if (rc)
> -		cifs_dbg(FYI, "SetPathInfo (file size) returned %d\n", rc);
> -
> -	cifs_buf_release(pSMB);
> -
> -	if (rc == -EAGAIN)
> -		goto SetEOFRetry;
> -
> -	return rc;
> -}
> -
>  int
>  CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>  		   struct cifsFileInfo *cfile, __u64 size, bool set_allocation)
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 5f5ba0d..14d4832 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -756,6 +756,40 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> +smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +	      const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb,
> +	      bool set_allocation)
> +{
> +	int oplock = 0;
> +	int rc;
> +	__u16 netfid;
> +	struct cifsFileInfo cfile;
> +
> +	cifs_dbg(FYI, "smb_set_file_size: %s\n", full_path);
> +
> +	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> +			 FILE_WRITE_DATA, CREATE_NOT_DIR,
> +			 &netfid, &oplock, NULL, cifs_sb->local_nls,
> +			 cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Only 2 fields are required to set the file size.
> +	 */
> +	memset(&cfile, 0, sizeof(cfile));
> +	cfile.pid = current->tgid;
> +	cfile.fid.netfid = netfid;
> +
> +	rc = CIFSSMBSetFileSize(xid, tcon, &cfile, size, set_allocation);
> +
> +	if (!rc)
> +		rc = CIFSSMBClose(xid, tcon, netfid);
> +
> +	return rc;
> +}
> +
> +static int
>  smb_set_file_info(struct inode *inode, const char *full_path,
>  		  FILE_BASIC_INFO *buf, const unsigned int xid)
>  {
> @@ -979,7 +1013,7 @@ struct smb_version_operations smb1_operations = {
>  	.query_path_info = cifs_query_path_info,
>  	.query_file_info = cifs_query_file_info,
>  	.get_srv_inum = cifs_get_srv_inum,
> -	.set_path_size = CIFSSMBSetEOF,
> +	.set_path_size = smb_set_file_size,
>  	.set_file_size = CIFSSMBSetFileSize,
>  	.set_file_info = smb_set_file_info,
>  	.set_compression = cifs_set_compression,

The basic idea looks correct, though the fact that this now becomes
3 round trips to the server sort of sucks. I don't see a way to avoid
that though.

Now that I look though, it's clear to me that cifs_set_file_size is
just wrong. Currently it calls ops->set_path_size and if that fails
with certain error codes it tries to do a SMBLegacyOpen, etc. That's
going to fall on its face if this is a SMB2/3 connection.

Here's what should be done instead, I think:

- get rid of both set_path_size and set_file_size operations. The
  version specific abstraction was implemented at the wrong level, IMO.

- implement a new protocol level operation that basically takes the
  same args as cifs_set_file_size. cifs_setattr_unix/_nounix should
  call this operation.

- the set_path_size operation for SMB1 should basically be the function
  that is cifs_set_file_size today (though that should probably be
  renamed). That function should be restructured to do the following:

  + look for an open filehandle
  + if there isn't one, open the file
  + call CIFSSMBSetFileSize
  + fall back to zero-length write if that fails
  + close the file if we opened it

Sound reasonable?
-- 
Jeff Layton <jlayton at redhat.com>


More information about the samba-technical mailing list