diff mbox series

[v2,12/16] fs: move permission hook out of do_iter_read()

Message ID 20231122122715.2561213-13-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 
We recently moved fsnotify hook, rw_verify_area() and other checks from
do_iter_write() out to its two callers.

for consistency, do the same thing for do_iter_read() - move the
rw_verify_area() checks and fsnotify hook to the callers vfs_iter_read()
and vfs_readv().

This aligns those vfs helpers with the pattern used in vfs_read() and
vfs_iocb_iter_read() and the vfs write helpers, where all the checks are
in the vfs helpers and the do_* or call_* helpers do the work.

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 | 74 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 26 deletions(-)

Comments

Jan Kara Nov. 23, 2023, 5:13 p.m. UTC | #1 
On Wed 22-11-23 14:27:11, Amir Goldstein wrote:
> We recently moved fsnotify hook, rw_verify_area() and other checks from
> do_iter_write() out to its two callers.
> 
> for consistency, do the same thing for do_iter_read() - move the
  ^^^ For

> rw_verify_area() checks and fsnotify hook to the callers vfs_iter_read()
> and vfs_readv().
> 
> This aligns those vfs helpers with the pattern used in vfs_read() and
> vfs_iocb_iter_read() and the vfs write helpers, where all the checks are
> in the vfs helpers and the do_* or call_* helpers do the work.
> 
> 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>

Also one nit below. Otherwise feel free to add:

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

> +/*
> + * Low-level helpers don't perform rw sanity checks.
> + * The caller is responsible for that.
> + */
>  static ssize_t do_iter_read(struct file *file, struct iov_iter *iter,
> -		loff_t *pos, rwf_t flags)
> +			    loff_t *pos, rwf_t flags)
> +{
> +	if (file->f_op->read_iter)
> +		return do_iter_readv_writev(file, iter, pos, READ, flags);
> +
> +	return do_loop_readv_writev(file, iter, pos, READ, flags);
> +}

Similarly as with do_iter_write() I don't think there's much point in this
helper when there's actually only one real user, the other one can just
call do_iter_readv_writev().

								Honza
Amir Goldstein Nov. 24, 2023, 8:48 a.m. UTC | #2 
On Fri, Nov 24, 2023 at 6:21 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 22-11-23 14:27:11, Amir Goldstein wrote:
> > We recently moved fsnotify hook, rw_verify_area() and other checks from
> > do_iter_write() out to its two callers.
> >
> > for consistency, do the same thing for do_iter_read() - move the
>   ^^^ For
>
> > rw_verify_area() checks and fsnotify hook to the callers vfs_iter_read()
> > and vfs_readv().
> >
> > This aligns those vfs helpers with the pattern used in vfs_read() and
> > vfs_iocb_iter_read() and the vfs write helpers, where all the checks are
> > in the vfs helpers and the do_* or call_* helpers do the work.
> >
> > 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>
>
> Also one nit below. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> > +/*
> > + * Low-level helpers don't perform rw sanity checks.
> > + * The caller is responsible for that.
> > + */
> >  static ssize_t do_iter_read(struct file *file, struct iov_iter *iter,
> > -             loff_t *pos, rwf_t flags)
> > +                         loff_t *pos, rwf_t flags)
> > +{
> > +     if (file->f_op->read_iter)
> > +             return do_iter_readv_writev(file, iter, pos, READ, flags);
> > +
> > +     return do_loop_readv_writev(file, iter, pos, READ, flags);
> > +}
>
> Similarly as with do_iter_write() I don't think there's much point in this
> helper when there's actually only one real user, the other one can just
> call do_iter_readv_writev().

Yeh, nice.

Christian,

Can you please fold this patch:

BTW, both patches need a very mild edit of commit message
to mention removal of do_iter_{read/write}() and the minor
spelling mistake fixes pointed out by Jan.

Thanks,
Amir.

diff --git a/fs/read_write.c b/fs/read_write.c
index 8358ace9282e..2953fea9b65b 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -784,19 +784,6 @@ static ssize_t do_loop_readv_writev(struct file
*filp, struct iov_iter *iter,
        return ret;
 }

-/*
- * Low-level helpers don't perform rw sanity checks.
- * The caller is responsible for that.
- */
-static ssize_t do_iter_read(struct file *file, struct iov_iter *iter,
-                           loff_t *pos, rwf_t flags)
-{
-       if (file->f_op->read_iter)
-               return do_iter_readv_writev(file, iter, pos, READ, flags);
-
-       return do_loop_readv_writev(file, iter, pos, READ, flags);
-}
-
 ssize_t vfs_iocb_iter_read(struct file *file, struct kiocb *iocb,
                           struct iov_iter *iter)
 {
@@ -845,7 +832,7 @@ ssize_t vfs_iter_read(struct file *file, struct
iov_iter *iter, loff_t *ppos,
        if (ret < 0)
                return ret;

-       ret = do_iter_read(file, iter, ppos, flags);
+       ret = do_iter_readv_writev(file, iter, ppos, READ, flags);
 out:
        if (ret >= 0)
                fsnotify_access(file);
@@ -939,7 +926,10 @@ static ssize_t vfs_readv(struct file *file, const
struct iovec __user *vec,
        if (ret < 0)
                goto out;

-       ret = do_iter_read(file, &iter, pos, flags);
+       if (file->f_op->read_iter)
+               ret = do_iter_readv_writev(file, &iter, pos, READ, flags);
+       else
+               ret = do_loop_readv_writev(file, &iter, pos, READ, flags);
 out:
        if (ret >= 0)
                fsnotify_access(file);
--
diff mbox series

Patch

diff --git a/fs/read_write.c b/fs/read_write.c
index 6c40468efe19..9410c3e6a04e 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -784,12 +784,27 @@  static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter,
 	return ret;
 }
 
+/*
+ * Low-level helpers don't perform rw sanity checks.
+ * The caller is responsible for that.
+ */
 static ssize_t do_iter_read(struct file *file, struct iov_iter *iter,
-		loff_t *pos, rwf_t flags)
+			    loff_t *pos, rwf_t flags)
+{
+	if (file->f_op->read_iter)
+		return do_iter_readv_writev(file, iter, pos, READ, flags);
+
+	return do_loop_readv_writev(file, iter, pos, READ, flags);
+}
+
+ssize_t vfs_iocb_iter_read(struct file *file, struct kiocb *iocb,
+			   struct iov_iter *iter)
 {
 	size_t tot_len;
 	ssize_t ret = 0;
 
+	if (!file->f_op->read_iter)
+		return -EINVAL;
 	if (!(file->f_mode & FMODE_READ))
 		return -EBADF;
 	if (!(file->f_mode & FMODE_CAN_READ))
@@ -798,22 +813,20 @@  static ssize_t do_iter_read(struct file *file, struct iov_iter *iter,
 	tot_len = iov_iter_count(iter);
 	if (!tot_len)
 		goto out;
-	ret = rw_verify_area(READ, file, pos, tot_len);
+	ret = rw_verify_area(READ, file, &iocb->ki_pos, tot_len);
 	if (ret < 0)
 		return ret;
 
-	if (file->f_op->read_iter)
-		ret = do_iter_readv_writev(file, iter, pos, READ, flags);
-	else
-		ret = do_loop_readv_writev(file, iter, pos, READ, flags);
+	ret = call_read_iter(file, iocb, iter);
 out:
 	if (ret >= 0)
 		fsnotify_access(file);
 	return ret;
 }
+EXPORT_SYMBOL(vfs_iocb_iter_read);
 
-ssize_t vfs_iocb_iter_read(struct file *file, struct kiocb *iocb,
-			   struct iov_iter *iter)
+ssize_t vfs_iter_read(struct file *file, struct iov_iter *iter, loff_t *ppos,
+		      rwf_t flags)
 {
 	size_t tot_len;
 	ssize_t ret = 0;
@@ -828,25 +841,16 @@  ssize_t vfs_iocb_iter_read(struct file *file, struct kiocb *iocb,
 	tot_len = iov_iter_count(iter);
 	if (!tot_len)
 		goto out;
-	ret = rw_verify_area(READ, file, &iocb->ki_pos, tot_len);
+	ret = rw_verify_area(READ, file, ppos, tot_len);
 	if (ret < 0)
 		return ret;
 
-	ret = call_read_iter(file, iocb, iter);
+	ret = do_iter_read(file, iter, ppos, flags);
 out:
 	if (ret >= 0)
 		fsnotify_access(file);
 	return ret;
 }
-EXPORT_SYMBOL(vfs_iocb_iter_read);
-
-ssize_t vfs_iter_read(struct file *file, struct iov_iter *iter, loff_t *ppos,
-		rwf_t flags)
-{
-	if (!file->f_op->read_iter)
-		return -EINVAL;
-	return do_iter_read(file, iter, ppos, flags);
-}
 EXPORT_SYMBOL(vfs_iter_read);
 
 static ssize_t do_iter_write(struct file *file, struct iov_iter *iter,
@@ -918,19 +922,37 @@  ssize_t vfs_iter_write(struct file *file, struct iov_iter *iter, loff_t *ppos,
 EXPORT_SYMBOL(vfs_iter_write);
 
 static ssize_t vfs_readv(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_DEST, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
-	if (ret >= 0) {
-		ret = do_iter_read(file, &iter, pos, flags);
-		kfree(iov);
-	}
+	if (!(file->f_mode & FMODE_READ))
+		return -EBADF;
+	if (!(file->f_mode & FMODE_CAN_READ))
+		return -EINVAL;
 
+	ret = import_iovec(ITER_DEST, 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(READ, file, pos, tot_len);
+	if (ret < 0)
+		goto out;
+
+	ret = do_iter_read(file, &iter, pos, flags);
+out:
+	if (ret >= 0)
+		fsnotify_access(file);
+	kfree(iov);
 	return ret;
 }