For backing file operations, users are expected to pass credentials
that will outlive the backing file common operations.
Use the specialized guard statements to override/revert the
credentials.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
fs/backing-file.c | 124 ++++++++++++++++++++++------------------------
1 file changed, 60 insertions(+), 64 deletions(-)
diff --git a/fs/backing-file.c b/fs/backing-file.c
index a681f38d84d8..9874f09f860f 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -140,7 +140,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
struct backing_file_ctx *ctx)
{
struct backing_aio *aio = NULL;
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
@@ -153,29 +153,28 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
!(file->f_mode & FMODE_CAN_ODIRECT))
return -EINVAL;
- old_cred = override_creds(ctx->cred);
- if (is_sync_kiocb(iocb)) {
- rwf_t rwf = iocb_to_rw_flags(flags);
+ scoped_guard(cred, old_cred) {
+ if (is_sync_kiocb(iocb)) {
+ rwf_t rwf = iocb_to_rw_flags(flags);
- ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
- } else {
- ret = -ENOMEM;
- aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
- if (!aio)
- goto out;
+ ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
+ } else {
+ ret = -ENOMEM;
+ aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
+ if (!aio)
+ goto out;
- aio->orig_iocb = iocb;
- kiocb_clone(&aio->iocb, iocb, get_file(file));
- aio->iocb.ki_complete = backing_aio_rw_complete;
- refcount_set(&aio->ref, 2);
- ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
- backing_aio_put(aio);
- if (ret != -EIOCBQUEUED)
- backing_aio_cleanup(aio, ret);
+ aio->orig_iocb = iocb;
+ kiocb_clone(&aio->iocb, iocb, get_file(file));
+ aio->iocb.ki_complete = backing_aio_rw_complete;
+ refcount_set(&aio->ref, 2);
+ ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
+ backing_aio_put(aio);
+ if (ret != -EIOCBQUEUED)
+ backing_aio_cleanup(aio, ret);
+ }
}
out:
- revert_creds(old_cred);
-
if (ctx->accessed)
ctx->accessed(ctx->user_file);
@@ -187,7 +186,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
struct kiocb *iocb, int flags,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
@@ -210,39 +209,37 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
*/
flags &= ~IOCB_DIO_CALLER_COMP;
- old_cred = override_creds(ctx->cred);
- if (is_sync_kiocb(iocb)) {
- rwf_t rwf = iocb_to_rw_flags(flags);
+ scoped_guard(cred, old_cred) {
+ if (is_sync_kiocb(iocb)) {
+ rwf_t rwf = iocb_to_rw_flags(flags);
- ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
- if (ctx->end_write)
- ctx->end_write(ctx->user_file);
- } else {
- struct backing_aio *aio;
+ ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
+ if (ctx->end_write)
+ ctx->end_write(ctx->user_file);
+ } else {
+ struct backing_aio *aio;
- ret = backing_aio_init_wq(iocb);
- if (ret)
- goto out;
+ ret = backing_aio_init_wq(iocb);
+ if (ret)
+ return ret;
- ret = -ENOMEM;
- aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
- if (!aio)
- goto out;
+ ret = -ENOMEM;
+ aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
+ if (!aio)
+ return ret;
- aio->orig_iocb = iocb;
- aio->end_write = ctx->end_write;
- kiocb_clone(&aio->iocb, iocb, get_file(file));
- aio->iocb.ki_flags = flags;
- aio->iocb.ki_complete = backing_aio_queue_completion;
- refcount_set(&aio->ref, 2);
- ret = vfs_iocb_iter_write(file, &aio->iocb, iter);
- backing_aio_put(aio);
- if (ret != -EIOCBQUEUED)
- backing_aio_cleanup(aio, ret);
+ aio->orig_iocb = iocb;
+ aio->end_write = ctx->end_write;
+ kiocb_clone(&aio->iocb, iocb, get_file(file));
+ aio->iocb.ki_flags = flags;
+ aio->iocb.ki_complete = backing_aio_queue_completion;
+ refcount_set(&aio->ref, 2);
+ ret = vfs_iocb_iter_write(file, &aio->iocb, iter);
+ backing_aio_put(aio);
+ if (ret != -EIOCBQUEUED)
+ backing_aio_cleanup(aio, ret);
+ }
}
-out:
- revert_creds(old_cred);
-
return ret;
}
EXPORT_SYMBOL_GPL(backing_file_write_iter);
@@ -252,15 +249,15 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
unsigned int flags,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(in->f_mode & FMODE_BACKING)))
return -EIO;
- old_cred = override_creds(ctx->cred);
- ret = vfs_splice_read(in, ppos, pipe, len, flags);
- revert_creds(old_cred);
+ scoped_guard(cred, old_cred) {
+ ret = vfs_splice_read(in, ppos, pipe, len, flags);
+ }
if (ctx->accessed)
ctx->accessed(ctx->user_file);
@@ -274,7 +271,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
unsigned int flags,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(out->f_mode & FMODE_BACKING)))
@@ -284,12 +281,11 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
if (ret)
return ret;
- old_cred = override_creds(ctx->cred);
- file_start_write(out);
- ret = iter_file_splice_write(pipe, out, ppos, len, flags);
- file_end_write(out);
- revert_creds(old_cred);
-
+ scoped_guard(cred, old_cred) {
+ file_start_write(out);
+ ret = iter_file_splice_write(pipe, out, ppos, len, flags);
+ file_end_write(out);
+ }
if (ctx->end_write)
ctx->end_write(ctx->user_file);
@@ -300,7 +296,7 @@ EXPORT_SYMBOL_GPL(backing_file_splice_write);
int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
int ret;
if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
@@ -312,9 +308,9 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
vma_set_file(vma, file);
- old_cred = override_creds(ctx->cred);
- ret = call_mmap(vma->vm_file, vma);
- revert_creds(old_cred);
+ scoped_guard(cred, old_cred) {
+ ret = call_mmap(vma->vm_file, vma);
+ }
if (ctx->accessed)
ctx->accessed(ctx->user_file);
--
2.43.0On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> For backing file operations, users are expected to pass credentials
> that will outlive the backing file common operations.
>
> Use the specialized guard statements to override/revert the
> credentials.
>
As I wrote before, I prefer to see this patch gets reviewed and merged
before the overlayfs large patch, so please reorder the series.
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> fs/backing-file.c | 124 ++++++++++++++++++++++------------------------
> 1 file changed, 60 insertions(+), 64 deletions(-)
>
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index a681f38d84d8..9874f09f860f 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -140,7 +140,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
> struct backing_file_ctx *ctx)
> {
> struct backing_aio *aio = NULL;
> - const struct cred *old_cred;
> + const struct cred *old_cred = ctx->cred;
> ssize_t ret;
>
> if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
> @@ -153,29 +153,28 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
> !(file->f_mode & FMODE_CAN_ODIRECT))
> return -EINVAL;
>
> - old_cred = override_creds(ctx->cred);
> - if (is_sync_kiocb(iocb)) {
> - rwf_t rwf = iocb_to_rw_flags(flags);
> + scoped_guard(cred, old_cred) {
This reads very strage.
Also, I see that e.g. scoped_guard(spinlock_irqsave, ... hides the local var
used for save/restore of flags inside the macro.
Perhaps you use the same technique for scoped_guard(cred, ..
loose the local old_cred variable in all those functions and then the
code will read:
scoped_guard(cred, ctx->cred) {
which is nicer IMO.
> + if (is_sync_kiocb(iocb)) {
> + rwf_t rwf = iocb_to_rw_flags(flags);
>
> - ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
> - } else {
> - ret = -ENOMEM;
> - aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
> - if (!aio)
> - goto out;
> + ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
> + } else {
> + ret = -ENOMEM;
> + aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
> + if (!aio)
> + goto out;
>
> - aio->orig_iocb = iocb;
> - kiocb_clone(&aio->iocb, iocb, get_file(file));
> - aio->iocb.ki_complete = backing_aio_rw_complete;
> - refcount_set(&aio->ref, 2);
> - ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
> - backing_aio_put(aio);
> - if (ret != -EIOCBQUEUED)
> - backing_aio_cleanup(aio, ret);
> + aio->orig_iocb = iocb;
> + kiocb_clone(&aio->iocb, iocb, get_file(file));
> + aio->iocb.ki_complete = backing_aio_rw_complete;
> + refcount_set(&aio->ref, 2);
> + ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
> + backing_aio_put(aio);
> + if (ret != -EIOCBQUEUED)
> + backing_aio_cleanup(aio, ret);
> + }
if possible, I would rather avoid all this churn in functions that mostly
do work with the new cred, so either use guard(cred, ) directly or split a
helper that uses guard(cred, ) form the rest.
Thanks,
Amir.
Amir Goldstein <amir73il@gmail.com> writes:
> On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> For backing file operations, users are expected to pass credentials
>> that will outlive the backing file common operations.
>>
>> Use the specialized guard statements to override/revert the
>> credentials.
>>
>
> As I wrote before, I prefer to see this patch gets reviewed and merged
> before the overlayfs large patch, so please reorder the series.
>
Sure. Will do.
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>> fs/backing-file.c | 124 ++++++++++++++++++++++------------------------
>> 1 file changed, 60 insertions(+), 64 deletions(-)
>>
>> diff --git a/fs/backing-file.c b/fs/backing-file.c
>> index a681f38d84d8..9874f09f860f 100644
>> --- a/fs/backing-file.c
>> +++ b/fs/backing-file.c
>> @@ -140,7 +140,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
>> struct backing_file_ctx *ctx)
>> {
>> struct backing_aio *aio = NULL;
>> - const struct cred *old_cred;
>> + const struct cred *old_cred = ctx->cred;
>> ssize_t ret;
>>
>> if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
>> @@ -153,29 +153,28 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
>> !(file->f_mode & FMODE_CAN_ODIRECT))
>> return -EINVAL;
>>
>> - old_cred = override_creds(ctx->cred);
>> - if (is_sync_kiocb(iocb)) {
>> - rwf_t rwf = iocb_to_rw_flags(flags);
>> + scoped_guard(cred, old_cred) {
>
> This reads very strage.
>
> Also, I see that e.g. scoped_guard(spinlock_irqsave, ... hides the local var
> used for save/restore of flags inside the macro.
>
> Perhaps you use the same technique for scoped_guard(cred, ..
> loose the local old_cred variable in all those functions and then the
> code will read:
>
> scoped_guard(cred, ctx->cred) {
>
> which is nicer IMO.
Most likely using DEFINE_LOCK_GUARD_1() would allow us to use the nicer version.
>
>> + if (is_sync_kiocb(iocb)) {
>> + rwf_t rwf = iocb_to_rw_flags(flags);
>>
>> - ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
>> - } else {
>> - ret = -ENOMEM;
>> - aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
>> - if (!aio)
>> - goto out;
>> + ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
>> + } else {
>> + ret = -ENOMEM;
>> + aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
>> + if (!aio)
>> + goto out;
>>
>> - aio->orig_iocb = iocb;
>> - kiocb_clone(&aio->iocb, iocb, get_file(file));
>> - aio->iocb.ki_complete = backing_aio_rw_complete;
>> - refcount_set(&aio->ref, 2);
>> - ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
>> - backing_aio_put(aio);
>> - if (ret != -EIOCBQUEUED)
>> - backing_aio_cleanup(aio, ret);
>> + aio->orig_iocb = iocb;
>> + kiocb_clone(&aio->iocb, iocb, get_file(file));
>> + aio->iocb.ki_complete = backing_aio_rw_complete;
>> + refcount_set(&aio->ref, 2);
>> + ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
>> + backing_aio_put(aio);
>> + if (ret != -EIOCBQUEUED)
>> + backing_aio_cleanup(aio, ret);
>> + }
>
> if possible, I would rather avoid all this churn in functions that mostly
> do work with the new cred, so either use guard(cred, ) directly or split a
> helper that uses guard(cred, ) form the rest.
>
Yeah, I think what happened is that I tried to keep the scope of the
guard to be as close as possible to override/revert (as you said), and
that caused the churn.
Probably using guard() more will reduce these confusing code changes. I
am going to try that.
> Thanks,
> Amir.
Cheers,
--
Vinicius
© 2016 - 2025 Red Hat, Inc.