diff mbox series

[v2,11/16] fs: move permission hook out of do_iter_write()

Message ID 20231122122715.2561213-12-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series Tidy up file permission hooks | expand

Commit Message

Amir Goldstein Nov. 22, 2023, 12:27 p.m. UTC 
In many of the vfs helpers, the rw_verity_area() checks are called before
taking sb_start_write(), making them "start-write-safe".
do_iter_write() is an exception to this rule.

do_iter_write() has two callers - vfs_iter_write() and vfs_writev().
Move rw_verify_area() and other checks from do_iter_write() out to
its callers to make them "start-write-safe".

Move also the fsnotify_modify() hook to align with similar pattern
used in vfs_write() and other vfs helpers.

This is needed for fanotify "pre content" events.

Suggested-by: Jan Kara <jack@suse.cz>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/read_write.c | 78 +++++++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 31 deletions(-)

Comments

Christian Brauner Nov. 22, 2023, 2:33 p.m. UTC | #1 
> -	ret = import_iovec(ITER_SOURCE, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
> -	if (ret >= 0) {
> -		file_start_write(file);
> -		ret = do_iter_write(file, &iter, pos, flags);
> -		file_end_write(file);
> -		kfree(iov);
> -	}
> +	if (!(file->f_mode & FMODE_WRITE))
> +		return -EBADF;
> +	if (!(file->f_mode & FMODE_CAN_WRITE))
> +		return -EINVAL;
> +
> +	ret = import_iovec(ITER_SOURCE, vec, vlen, ARRAY_SIZE(iovstack), &iov,
> +			   &iter);
> +	if (ret < 0)
> +		return ret;
> +
> +	tot_len = iov_iter_count(&iter);
> +	if (!tot_len)
> +		goto out;

Fwiw, the logic is slightly changed here. This now relies on
import_iovec() >= 0 then iov_iter_count() >= 0.

If that's ever changed and iov_iter_count() can return an error even
though import_iovec() succeeded we'll be returning the number of
imported bytes even though nothing was written and also generate a
fsnotify event because ret still points to the number of imported bytes.

The way it was written before it didn't matter because this was hidden
in a function call that returned 0 and initialized ret again. Anyway, I
can just massage that in-tree if that's worth it. Nothing to do for you.
Amir Goldstein Nov. 22, 2023, 3:25 p.m. UTC | #2 
On Wed, Nov 22, 2023 at 4:34 PM Christian Brauner <brauner@kernel.org> wrote:
>
> > -     ret = import_iovec(ITER_SOURCE, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
> > -     if (ret >= 0) {
> > -             file_start_write(file);
> > -             ret = do_iter_write(file, &iter, pos, flags);
> > -             file_end_write(file);
> > -             kfree(iov);
> > -     }
> > +     if (!(file->f_mode & FMODE_WRITE))
> > +             return -EBADF;
> > +     if (!(file->f_mode & FMODE_CAN_WRITE))
> > +             return -EINVAL;
> > +
> > +     ret = import_iovec(ITER_SOURCE, vec, vlen, ARRAY_SIZE(iovstack), &iov,
> > +                        &iter);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     tot_len = iov_iter_count(&iter);
> > +     if (!tot_len)
> > +             goto out;
>
> Fwiw, the logic is slightly changed here. This now relies on
> import_iovec() >= 0 then iov_iter_count() >= 0.
>
> If that's ever changed and iov_iter_count() can return an error even
> though import_iovec() succeeded we'll be returning the number of
> imported bytes even though nothing was written and also generate a
> fsnotify event because ret still points to the number of imported bytes.
>
> The way it was written before it didn't matter because this was hidden
> in a function call that returned 0 and initialized ret again.

Good catch!

> Anyway, I
> can just massage that in-tree if that's worth it. Nothing to do for you.

Thank you!
Amir.
Jan Kara Nov. 23, 2023, 5:08 p.m. UTC | #3 
On Wed 22-11-23 14:27:10, Amir Goldstein wrote:
> In many of the vfs helpers, the rw_verity_area() checks are called before
					^^ verify

> taking sb_start_write(), making them "start-write-safe".
> do_iter_write() is an exception to this rule.
> 
> do_iter_write() has two callers - vfs_iter_write() and vfs_writev().
> Move rw_verify_area() and other checks from do_iter_write() out to
> its callers to make them "start-write-safe".
> 
> Move also the fsnotify_modify() hook to align with similar pattern
> used in vfs_write() and other vfs helpers.
> 
> This is needed for fanotify "pre content" events.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Just one more nit below. Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>


> diff --git a/fs/read_write.c b/fs/read_write.c
> index 87ca50f16a23..6c40468efe19 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -852,28 +852,10 @@ EXPORT_SYMBOL(vfs_iter_read);
>  static ssize_t do_iter_write(struct file *file, struct iov_iter *iter,
>  			     loff_t *pos, rwf_t flags)
>  {
> -	size_t tot_len;
> -	ssize_t ret = 0;
> -
> -	if (!(file->f_mode & FMODE_WRITE))
> -		return -EBADF;
> -	if (!(file->f_mode & FMODE_CAN_WRITE))
> -		return -EINVAL;
> -
> -	tot_len = iov_iter_count(iter);
> -	if (!tot_len)
> -		return 0;
> -	ret = rw_verify_area(WRITE, file, pos, tot_len);
> -	if (ret < 0)
> -		return ret;
> -
>  	if (file->f_op->write_iter)
> -		ret = do_iter_readv_writev(file, iter, pos, WRITE, flags);
> -	else
> -		ret = do_loop_readv_writev(file, iter, pos, WRITE, flags);
> -	if (ret > 0)
> -		fsnotify_modify(file);
> -	return ret;
> +		return do_iter_readv_writev(file, iter, pos, WRITE, flags);
> +
> +	return do_loop_readv_writev(file, iter, pos, WRITE, flags);
>  }

Since do_iter_write() is now trivial and has only two callers, one of which
has made sure file->f_op->write_iter != NULL, can we perhaps just fold this 
into the callers? One less wrapper in the maze...

								Honza
Amir Goldstein Nov. 24, 2023, 8:45 a.m. UTC | #4 
On Fri, Nov 24, 2023 at 6:18 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 22-11-23 14:27:10, Amir Goldstein wrote:
> > In many of the vfs helpers, the rw_verity_area() checks are called before
>                                         ^^ verify
>
> > taking sb_start_write(), making them "start-write-safe".
> > do_iter_write() is an exception to this rule.
> >
> > do_iter_write() has two callers - vfs_iter_write() and vfs_writev().
> > Move rw_verify_area() and other checks from do_iter_write() out to
> > its callers to make them "start-write-safe".
> >
> > Move also the fsnotify_modify() hook to align with similar pattern
> > used in vfs_write() and other vfs helpers.
> >
> > This is needed for fanotify "pre content" events.
> >
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Just one more nit below. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
>
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 87ca50f16a23..6c40468efe19 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -852,28 +852,10 @@ EXPORT_SYMBOL(vfs_iter_read);
> >  static ssize_t do_iter_write(struct file *file, struct iov_iter *iter,
> >                            loff_t *pos, rwf_t flags)
> >  {
> > -     size_t tot_len;
> > -     ssize_t ret = 0;
> > -
> > -     if (!(file->f_mode & FMODE_WRITE))
> > -             return -EBADF;
> > -     if (!(file->f_mode & FMODE_CAN_WRITE))
> > -             return -EINVAL;
> > -
> > -     tot_len = iov_iter_count(iter);
> > -     if (!tot_len)
> > -             return 0;
> > -     ret = rw_verify_area(WRITE, file, pos, tot_len);
> > -     if (ret < 0)
> > -             return ret;
> > -
> >       if (file->f_op->write_iter)
> > -             ret = do_iter_readv_writev(file, iter, pos, WRITE, flags);
> > -     else
> > -             ret = do_loop_readv_writev(file, iter, pos, WRITE, flags);
> > -     if (ret > 0)
> > -             fsnotify_modify(file);
> > -     return ret;
> > +             return do_iter_readv_writev(file, iter, pos, WRITE, flags);
> > +
> > +     return do_loop_readv_writev(file, iter, pos, WRITE, flags);
> >  }
>
> Since do_iter_write() is now trivial and has only two callers, one of which
> has made sure file->f_op->write_iter != NULL, can we perhaps just fold this
> into the callers? One less wrapper in the maze...

Yeh, nice cleanup.

Cristian,

Can you please fold this patch:

diff --git a/fs/read_write.c b/fs/read_write.c
index 6c40468efe19..46a90aa0ad56 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -849,15 +849,6 @@ ssize_t vfs_iter_read(struct file *file, struct
iov_iter *iter, loff_t *ppos,
 }
 EXPORT_SYMBOL(vfs_iter_read);

-static ssize_t do_iter_write(struct file *file, struct iov_iter *iter,
-                            loff_t *pos, rwf_t flags)
-{
-       if (file->f_op->write_iter)
-               return do_iter_readv_writev(file, iter, pos, WRITE, flags);
-
-       return do_loop_readv_writev(file, iter, pos, WRITE, flags);
-}
-
 ssize_t vfs_iocb_iter_write(struct file *file, struct kiocb *iocb,
                            struct iov_iter *iter)
 {
@@ -908,7 +899,7 @@ ssize_t vfs_iter_write(struct file *file, struct
iov_iter *iter, loff_t *ppos,
                return ret;

        file_start_write(file);
-       ret = do_iter_write(file, iter, ppos, flags);
+       ret = do_iter_readv_writev(file, iter, ppos, WRITE, flags);
        if (ret > 0)
                fsnotify_modify(file);
        file_end_write(file);
@@ -962,7 +953,10 @@ static ssize_t vfs_writev(struct file *file,
const struct iovec __user *vec,
                goto out;

        file_start_write(file);
-       ret = do_iter_write(file, &iter, pos, flags);
+       if (file->f_op->write_iter)
+               ret = do_iter_readv_writev(file, &iter, pos, WRITE, flags);
+       else
+               ret = do_loop_readv_writev(file, &iter, pos, WRITE, flags);
        if (ret > 0)
                fsnotify_modify(file);
        file_end_write(file);
--
diff mbox series

Patch

diff --git a/fs/read_write.c b/fs/read_write.c
index 87ca50f16a23..6c40468efe19 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -852,28 +852,10 @@  EXPORT_SYMBOL(vfs_iter_read);
 static ssize_t do_iter_write(struct file *file, struct iov_iter *iter,
 			     loff_t *pos, rwf_t flags)
 {
-	size_t tot_len;
-	ssize_t ret = 0;
-
-	if (!(file->f_mode & FMODE_WRITE))
-		return -EBADF;
-	if (!(file->f_mode & FMODE_CAN_WRITE))
-		return -EINVAL;
-
-	tot_len = iov_iter_count(iter);
-	if (!tot_len)
-		return 0;
-	ret = rw_verify_area(WRITE, file, pos, tot_len);
-	if (ret < 0)
-		return ret;
-
 	if (file->f_op->write_iter)
-		ret = do_iter_readv_writev(file, iter, pos, WRITE, flags);
-	else
-		ret = do_loop_readv_writev(file, iter, pos, WRITE, flags);
-	if (ret > 0)
-		fsnotify_modify(file);
-	return ret;
+		return do_iter_readv_writev(file, iter, pos, WRITE, flags);
+
+	return do_loop_readv_writev(file, iter, pos, WRITE, flags);
 }
 
 ssize_t vfs_iocb_iter_write(struct file *file, struct kiocb *iocb,
@@ -907,13 +889,28 @@  EXPORT_SYMBOL(vfs_iocb_iter_write);
 ssize_t vfs_iter_write(struct file *file, struct iov_iter *iter, loff_t *ppos,
 		       rwf_t flags)
 {
-	int ret;
+	size_t tot_len;
+	ssize_t ret;
 
+	if (!(file->f_mode & FMODE_WRITE))
+		return -EBADF;
+	if (!(file->f_mode & FMODE_CAN_WRITE))
+		return -EINVAL;
 	if (!file->f_op->write_iter)
 		return -EINVAL;
 
+	tot_len = iov_iter_count(iter);
+	if (!tot_len)
+		return 0;
+
+	ret = rw_verify_area(WRITE, file, ppos, tot_len);
+	if (ret < 0)
+		return ret;
+
 	file_start_write(file);
 	ret = do_iter_write(file, iter, ppos, flags);
+	if (ret > 0)
+		fsnotify_modify(file);
 	file_end_write(file);
 
 	return ret;
@@ -938,20 +935,39 @@  static ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
 }
 
 static ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
-		   unsigned long vlen, loff_t *pos, rwf_t flags)
+			  unsigned long vlen, loff_t *pos, rwf_t flags)
 {
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
 	struct iov_iter iter;
-	ssize_t ret;
+	size_t tot_len;
+	ssize_t ret = 0;
 
-	ret = import_iovec(ITER_SOURCE, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
-	if (ret >= 0) {
-		file_start_write(file);
-		ret = do_iter_write(file, &iter, pos, flags);
-		file_end_write(file);
-		kfree(iov);
-	}
+	if (!(file->f_mode & FMODE_WRITE))
+		return -EBADF;
+	if (!(file->f_mode & FMODE_CAN_WRITE))
+		return -EINVAL;
+
+	ret = import_iovec(ITER_SOURCE, vec, vlen, ARRAY_SIZE(iovstack), &iov,
+			   &iter);
+	if (ret < 0)
+		return ret;
+
+	tot_len = iov_iter_count(&iter);
+	if (!tot_len)
+		goto out;
+
+	ret = rw_verify_area(WRITE, file, pos, tot_len);
+	if (ret < 0)
+		goto out;
+
+	file_start_write(file);
+	ret = do_iter_write(file, &iter, pos, flags);
+	if (ret > 0)
+		fsnotify_modify(file);
+	file_end_write(file);
+out:
+	kfree(iov);
 	return ret;
 }