[PATCH][RFC] inode time update funnies in ncpfs

[PATCH][RFC] inode time update funnies in ncpfs

[Christoph Hellwig]

ncfpfs seems to update inode times by hand everywhere instead of using
the proper helpers.  This means:

 - the atime updates in mmap() and read() seems to miss various checks
   upodate_atime or one of the wrappers does.  Also it doesn't mark the
   inode dirty.
 - in write() you update mtime and _a_time instead of ctime as expected,
   also the usual checks and optimizations are missing.

In addition the fops contain some bogus checks like for a refular file
(but the fops are only used of ISREG files) and inode->i_sb although
that is guranteed to be non-zero.


--- 1.13/fs/ncpfs/file.c	2004-06-04 06:03:33 +02:00
+++ edited/fs/ncpfs/file.c	2004-08-18 16:16:27 +02:00
@@ -115,11 +115,6 @@
 
 	if (!ncp_conn_valid(NCP_SERVER(inode)))
 		return -EIO;
-	if (!S_ISREG(inode->i_mode)) {
-		DPRINTK("ncp_file_read: read from non-file, mode %07o\n",
-			inode->i_mode);
-		return -EINVAL;
-	}
 
 	pos = *ppos;
 
@@ -175,10 +170,8 @@
 
 	*ppos = pos;
 
-	if (!IS_RDONLY(inode)) {
-		inode->i_atime = CURRENT_TIME;
-	}
-	
+	file_accessed(file);
+
 	DPRINTK("ncp_file_read: exit %s/%s\n",
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 outrel:
@@ -201,11 +194,6 @@
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 	if (!ncp_conn_valid(NCP_SERVER(inode)))
 		return -EIO;
-	if (!S_ISREG(inode->i_mode)) {
-		DPRINTK("ncp_file_write: write to non-file, mode %07o\n",
-			inode->i_mode);
-		return -EINVAL;
-	}
 	if ((ssize_t) count < 0)
 		return -EINVAL;
 	pos = *ppos;
@@ -273,8 +261,9 @@
 		}
 	}
 	vfree(bouncebuffer);
-	inode->i_mtime = inode->i_atime = CURRENT_TIME;
-	
+
+	inode_update_time(inode, 1);
+
 	*ppos = pos;
 
 	if (pos > inode->i_size) {
===== fs/ncpfs/mmap.c 1.8 vs edited =====
--- 1.8/fs/ncpfs/mmap.c	2003-12-29 23:04:51 +01:00
+++ edited/fs/ncpfs/mmap.c	2004-08-18 16:06:59 +02:00
@@ -110,23 +110,19 @@
 	
 	DPRINTK("ncp_mmap: called\n");
 
-	if (!ncp_conn_valid(NCP_SERVER(inode))) {
+	if (!ncp_conn_valid(NCP_SERVER(inode)))
 		return -EIO;
-	}
+
 	/* only PAGE_COW or read-only supported now */
 	if (vma->vm_flags & VM_SHARED)
 		return -EINVAL;
-	if (!inode->i_sb || !S_ISREG(inode->i_mode))
-		return -EACCES;
 	/* we do not support files bigger than 4GB... We eventually 
 	   supports just 4GB... */
 	if (((vma->vm_end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff 
 	   > (1U << (32 - PAGE_SHIFT)))
 		return -EFBIG;
-	if (!IS_RDONLY(inode)) {
-		inode->i_atime = CURRENT_TIME;
-	}
 
 	vma->vm_ops = &ncp_file_mmap;
+	file_accessed(file);
 	return 0;
 }

Re: [PATCH][RFC] inode time update funnies in ncpfs

[Andrew Morton]

Christoph Hellwig <hch@lst.de> wrote:
>
> ncfpfs seems to update inode times by hand everywhere instead of using
> the proper helpers.  This means:
> 
>  - the atime updates in mmap() and read() seems to miss various checks
>    upodate_atime or one of the wrappers does.  Also it doesn't mark the
>    inode dirty.
>  - in write() you update mtime and _a_time instead of ctime as expected,
>    also the usual checks and optimizations are missing.
> 
> In addition the fops contain some bogus checks like for a refular file
> (but the fops are only used of ISREG files) and inode->i_sb although
> that is guranteed to be non-zero.
> 

Patch looks very correct to me - I'll merge it.

> --- 1.13/fs/ncpfs/file.c	2004-06-04 06:03:33 +02:00
> +++ edited/fs/ncpfs/file.c	2004-08-18 16:16:27 +02:00
> @@ -115,11 +115,6 @@
>  
>  	if (!ncp_conn_valid(NCP_SERVER(inode)))
>  		return -EIO;
> -	if (!S_ISREG(inode->i_mode)) {
> -		DPRINTK("ncp_file_read: read from non-file, mode %07o\n",
> -			inode->i_mode);
> -		return -EINVAL;
> -	}
>  
>  	pos = *ppos;
>  
> @@ -175,10 +170,8 @@
>  
>  	*ppos = pos;
>  
> -	if (!IS_RDONLY(inode)) {
> -		inode->i_atime = CURRENT_TIME;
> -	}
> -	
> +	file_accessed(file);
> +
>  	DPRINTK("ncp_file_read: exit %s/%s\n",
>  		dentry->d_parent->d_name.name, dentry->d_name.name);
>  outrel:
> @@ -201,11 +194,6 @@
>  		dentry->d_parent->d_name.name, dentry->d_name.name);
>  	if (!ncp_conn_valid(NCP_SERVER(inode)))
>  		return -EIO;
> -	if (!S_ISREG(inode->i_mode)) {
> -		DPRINTK("ncp_file_write: write to non-file, mode %07o\n",
> -			inode->i_mode);
> -		return -EINVAL;
> -	}
>  	if ((ssize_t) count < 0)
>  		return -EINVAL;
>  	pos = *ppos;
> @@ -273,8 +261,9 @@
>  		}
>  	}
>  	vfree(bouncebuffer);
> -	inode->i_mtime = inode->i_atime = CURRENT_TIME;
> -	
> +
> +	inode_update_time(inode, 1);
> +
>  	*ppos = pos;
>  
>  	if (pos > inode->i_size) {
> ===== fs/ncpfs/mmap.c 1.8 vs edited =====
> --- 1.8/fs/ncpfs/mmap.c	2003-12-29 23:04:51 +01:00
> +++ edited/fs/ncpfs/mmap.c	2004-08-18 16:06:59 +02:00
> @@ -110,23 +110,19 @@
>  	
>  	DPRINTK("ncp_mmap: called\n");
>  
> -	if (!ncp_conn_valid(NCP_SERVER(inode))) {
> +	if (!ncp_conn_valid(NCP_SERVER(inode)))
>  		return -EIO;
> -	}
> +
>  	/* only PAGE_COW or read-only supported now */
>  	if (vma->vm_flags & VM_SHARED)
>  		return -EINVAL;
> -	if (!inode->i_sb || !S_ISREG(inode->i_mode))
> -		return -EACCES;
>  	/* we do not support files bigger than 4GB... We eventually 
>  	   supports just 4GB... */
>  	if (((vma->vm_end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff 
>  	   > (1U << (32 - PAGE_SHIFT)))
>  		return -EFBIG;
> -	if (!IS_RDONLY(inode)) {
> -		inode->i_atime = CURRENT_TIME;
> -	}
>  
>  	vma->vm_ops = &ncp_file_mmap;
> +	file_accessed(file);
>  	return 0;
>  }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html