On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
<vinicius.gomes@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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.
