[v1,1/3] fs: Don't copy beyond the end of the file
diff mbox

Message ID 20170302160211.30451-2-kolga@netapp.com
State New
Headers show

Commit Message

Olga Kornievskaia March 2, 2017, 4:02 p.m. UTC
From: Anna Schumaker <Anna.Schumaker@Netapp.com>

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/read_write.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Darrick J. Wong March 2, 2017, 4:58 p.m. UTC | #1
On Thu, Mar 02, 2017 at 11:02:09AM -0500, Olga Kornievskaia wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
>  fs/read_write.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 5816d4c..1d9e305 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	if (unlikely(ret))
>  		return ret;
>  
> +	if (pos_in >= i_size_read(inode_in))
> +		return -EINVAL;
> +

This ought to go in vfs_clone_file_prep_inodes.

(He says, noticing that btrfs never got updated to use that validator...)

--D

>  	if (!(file_in->f_mode & FMODE_READ) ||
>  	    !(file_out->f_mode & FMODE_WRITE) ||
>  	    (file_out->f_flags & O_APPEND))
> -- 
> 1.8.3.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia March 2, 2017, 6:21 p.m. UTC | #2
> On Mar 2, 2017, at 11:58 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> On Thu, Mar 02, 2017 at 11:02:09AM -0500, Olga Kornievskaia wrote:
>> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> 
>> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
>> ---
>> fs/read_write.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 5816d4c..1d9e305 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>> 	if (unlikely(ret))
>> 		return ret;
>> 
>> +	if (pos_in >= i_size_read(inode_in))
>> +		return -EINVAL;
>> +
> 
> This ought to go in vfs_clone_file_prep_inodes.
> 
> (He says, noticing that btrfs never got updated to use that validator…)

I apologize I’m not fully understanding the suggestion here. How btrfs is related to the check that I’m suggesting for the copy_file_range().  I don’t see how it would fix the problem for the copy_file_range().

Is the suggestion that NFS’s clone implementation is suppose to call vfs_clone_file_prep_inodes() where the check would be added and thus because vfs_copy_file_range() first decides to call clone() instead of copy() then that check would be met?

> 
> --D
> 
>> 	if (!(file_in->f_mode & FMODE_READ) ||
>> 	    !(file_out->f_mode & FMODE_WRITE) ||
>> 	    (file_out->f_flags & O_APPEND))
>> -- 
>> 1.8.3.1
>> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong March 2, 2017, 6:40 p.m. UTC | #3
On Thu, Mar 02, 2017 at 01:21:49PM -0500, Olga Kornievskaia wrote:
> 
> > On Mar 2, 2017, at 11:58 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > 
> > On Thu, Mar 02, 2017 at 11:02:09AM -0500, Olga Kornievskaia wrote:
> >> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >> 
> >> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >> ---
> >> fs/read_write.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/fs/read_write.c b/fs/read_write.c
> >> index 5816d4c..1d9e305 100644
> >> --- a/fs/read_write.c
> >> +++ b/fs/read_write.c
> >> @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >> 	if (unlikely(ret))
> >> 		return ret;
> >> 
> >> +	if (pos_in >= i_size_read(inode_in))
> >> +		return -EINVAL;
> >> +
> > 
> > This ought to go in vfs_clone_file_prep_inodes.
> > 
> > (He says, noticing that btrfs never got updated to use that validator…)
> 
> I apologize I’m not fully understanding the suggestion here. How btrfs is
> related to the check that I’m suggesting for the copy_file_range().  I don’t
> see how it would fix the problem for the copy_file_range().

Never mind, I misread vfs_copy as vfs_clone and thought you were talking
about something else. :(

Sorry about the noise.

--D

> Is the suggestion that NFS’s clone implementation is suppose to call
> vfs_clone_file_prep_inodes() where the check would be added and thus
> because vfs_copy_file_range() first decides to call clone() instead of
> copy() then that check would be met?
> 
> > 
> > --D
> > 
> >> 	if (!(file_in->f_mode & FMODE_READ) ||
> >> 	    !(file_out->f_mode & FMODE_WRITE) ||
> >> 	    (file_out->f_flags & O_APPEND))
> >> -- 
> >> 1.8.3.1
> >> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig March 7, 2017, 11:43 p.m. UTC | #4
On Thu, Mar 02, 2017 at 10:40:57AM -0800, Darrick J. Wong wrote:
> Never mind, I misread vfs_copy as vfs_clone and thought you were talking
> about something else. :(
> 
> Sorry about the noise.

And either way we'll need test coverage for copy_file_range before we
do any more changes to it.  I'm sick and tired of this doctoring around
without having any test coverage.  I've asked for it again and again
and we've made very little progress despite having the code in the tree
for almost a year and a half.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia March 7, 2017, 11:46 p.m. UTC | #5
> On Mar 7, 2017, at 6:43 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Mar 02, 2017 at 10:40:57AM -0800, Darrick J. Wong wrote:
>> Never mind, I misread vfs_copy as vfs_clone and thought you were talking
>> about something else. :(
>> 
>> Sorry about the noise.
> 
> And either way we'll need test coverage for copy_file_range before we
> do any more changes to it.  I'm sick and tired of this doctoring around
> without having any test coverage.  I've asked for it again and again
> and we've made very little progress despite having the code in the tree
> for almost a year and a half.


Anna has added test cases to the xfstest suite. Is there something that’s missing? --
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig March 7, 2017, 11:50 p.m. UTC | #6
On Tue, Mar 07, 2017 at 06:46:55PM -0500, Olga Kornievskaia wrote:
> Anna has added test cases to the xfstest suite. Is there something that’s missing? 

No, we still don't have that.  Anna did one attempt (or maybe two) but
didn't seem to have time to follow up the review comments.  Maybe you
can give it a spin to make sure we at least have basic test coverage,
including the cases mentioned in the man page like this.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia March 8, 2017, 3:39 p.m. UTC | #7
> On Mar 7, 2017, at 6:50 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Tue, Mar 07, 2017 at 06:46:55PM -0500, Olga Kornievskaia wrote:
>> Anna has added test cases to the xfstest suite. Is there something that’s missing? 
> 
> No, we still don't have that.  Anna did one attempt (or maybe two) but
> didn't seem to have time to follow up the review comments.  Maybe you
> can give it a spin to make sure we at least have basic test coverage,
> including the cases mentioned in the man page like this.

I’m reading some mail archives and I see that Anna submitted patches for xfstests. The comment was to re-work it as a part of xfs_io command. She posted the patches for that. According to the announcement I see here: http://oss.sgi.com/archives/xfs/2016-08/msg00221.html it says it’s including the new copy_file_range command in xfs_io. I don’t run these tests so I don’t know but it look to me that the tests are in.

Can you please what I’m getting wrong?

Thank you.



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig March 8, 2017, 3:57 p.m. UTC | #8
On Wed, Mar 08, 2017 at 10:39:11AM -0500, Olga Kornievskaia wrote:
> 
> I’m reading some mail archives and I see that Anna submitted patches for xfstests. The comment was to re-work it as a part of xfs_io command. She posted the patches for that. According to the announcement I see here: http://oss.sgi.com/archives/xfs/2016-08/msg00221.html it says it’s including the new copy_file_range command in xfs_io. I don’t run these tests so I don’t know but it look to me that the tests are in.

That's xfsprogs and the xfs_io command.  Now that this step is done
someone (e.g. Anna or you) needs to send the patches to xfstests that
use this command.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/read_write.c b/fs/read_write.c
index 5816d4c..1d9e305 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1526,6 +1526,9 @@  ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (unlikely(ret))
 		return ret;
 
+	if (pos_in >= i_size_read(inode_in))
+		return -EINVAL;
+
 	if (!(file_in->f_mode & FMODE_READ) ||
 	    !(file_out->f_mode & FMODE_WRITE) ||
 	    (file_out->f_flags & O_APPEND))