Introduce fs_revocable_replace() to simplify the use of the revocable
APIs with file_operations.
The function should only be used after filp->f_op->open(). It assumes
the filp->private_data would be set only once in filp->f_op->open() and
wouldn't update in subsequent file operations.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v6:
- Use filp->private_data for the replacement context.
- Prevent file operations from calling if the resource has been revoked.
- Support only 1 resource again.
- Rename REVOCABLE_TRY_ACCESS_WITH() -> REVOCABLE_TRY_ACCESS_SCOPED().
- Use new REVOCABLE_TRY_ACCESS_WITH() if applicable.
v5: https://lore.kernel.org/chrome-platform/20251016054204.1523139-6-tzungbi@kernel.org
- Rename to "fs_revocable".
- Move the replacement context to struct file.
- Support multiple revocable providers.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-6-tzungbi@kernel.org
- New in the series.
fs/Makefile | 2 +-
fs/fs_revocable.c | 156 +++++++++++++++++++++++++++++++++++
include/linux/fs_revocable.h | 14 ++++
3 files changed, 171 insertions(+), 1 deletion(-)
create mode 100644 fs/fs_revocable.c
create mode 100644 include/linux/fs_revocable.h
diff --git a/fs/Makefile b/fs/Makefile
index a04274a3c854..f1e5d7b52781 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -16,7 +16,7 @@ obj-y := open.o read_write.o file_table.o super.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
fs_dirent.o fs_context.o fs_parser.o fsopen.o init.o \
kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o \
- file_attr.o
+ file_attr.o fs_revocable.o
obj-$(CONFIG_BUFFER_HEAD) += buffer.o mpage.o
obj-$(CONFIG_PROC_FS) += proc_namespace.o
diff --git a/fs/fs_revocable.c b/fs/fs_revocable.c
new file mode 100644
index 000000000000..9ffa71cb67ed
--- /dev/null
+++ b/fs/fs_revocable.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google LLC
+ *
+ * File operation replacement with Revocable
+ */
+
+#include <linux/cleanup.h>
+#include <linux/fs_revocable.h>
+#include <linux/poll.h>
+#include <linux/revocable.h>
+
+struct fops_replacement {
+ struct file *filp;
+ void *orig_private_data;
+ const struct file_operations *orig_fops;
+ struct file_operations fops;
+ struct revocable *rev;
+};
+
+/*
+ * Recover the private_data to its original one.
+ */
+static struct fops_replacement *_recover_private_data(struct file *filp)
+{
+ struct fops_replacement *fr = filp->private_data;
+
+ filp->private_data = fr->orig_private_data;
+ return fr;
+}
+
+/*
+ * Replace the private_data to fops_replacement.
+ */
+static void _replace_private_data(struct fops_replacement *fr)
+{
+ fr->filp->private_data = fr;
+}
+
+DEFINE_CLASS(fops_replacement, struct fops_replacement *,
+ _replace_private_data(_T), _recover_private_data(filp),
+ struct file *filp)
+
+static ssize_t fs_revocable_read(struct file *filp, char __user *buffer,
+ size_t length, loff_t *offset)
+{
+ void *any;
+ CLASS(fops_replacement, fr)(filp);
+
+ REVOCABLE_TRY_ACCESS_WITH(fr->rev, any);
+ if (!any)
+ return -ENODEV;
+
+ return fr->orig_fops->read(filp, buffer, length, offset);
+}
+
+static __poll_t fs_revocable_poll(struct file *filp, poll_table *wait)
+{
+ void *any;
+ CLASS(fops_replacement, fr)(filp);
+
+ REVOCABLE_TRY_ACCESS_WITH(fr->rev, any);
+ if (!any)
+ return -ENODEV;
+
+ return fr->orig_fops->poll(filp, wait);
+}
+
+static long fs_revocable_unlocked_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ void *any;
+ CLASS(fops_replacement, fr)(filp);
+
+ REVOCABLE_TRY_ACCESS_WITH(fr->rev, any);
+ if (!any)
+ return -ENODEV;
+
+ return fr->orig_fops->unlocked_ioctl(filp, cmd, arg);
+}
+
+static int fs_revocable_release(struct inode *inode, struct file *filp)
+{
+ struct fops_replacement *fr = _recover_private_data(filp);
+ int ret = 0;
+ void *any;
+
+ filp->f_op = fr->orig_fops;
+
+ if (!fr->orig_fops->release)
+ goto leave;
+
+ REVOCABLE_TRY_ACCESS_SCOPED(fr->rev, any) {
+ if (!any) {
+ ret = -ENODEV;
+ goto leave;
+ }
+
+ ret = fr->orig_fops->release(inode, filp);
+ }
+
+leave:
+ kfree(fr);
+ return ret;
+}
+
+/**
+ * fs_revocable_replace() - Replace the file operations to be revocable-aware.
+ * @rp: The revocable resource provider.
+ * @filp: The opening file.
+ *
+ * This replaces @filp->f_op to a set of wrappers. The wrappers return -ENODEV
+ * if the resource provided by @rp has been revoked. Note that it doesn't
+ * concern how the file operations access the resource but only care about if
+ * the resource is still available.
+ *
+ * This should only be used after @filp->f_op->open(). It assumes the
+ * @filp->private_data would be set only once in @filp->f_op->open() and wouldn't
+ * update in subsequent file operations.
+ */
+int fs_revocable_replace(struct revocable_provider *rp, struct file *filp)
+{
+ struct fops_replacement *fr;
+
+ fr = kzalloc(sizeof(*fr), GFP_KERNEL);
+ if (!fr)
+ return -ENOMEM;
+
+ fr->rev = revocable_alloc(rp);
+ if (!fr->rev)
+ goto free_fr;
+
+ fr->filp = filp;
+ fr->orig_private_data = filp->private_data;
+ fr->orig_fops = filp->f_op;
+
+ memcpy(&fr->fops, filp->f_op, sizeof(fr->fops));
+ fr->fops.release = fs_revocable_release;
+
+ if (fr->fops.read)
+ fr->fops.read = fs_revocable_read;
+ if (fr->fops.poll)
+ fr->fops.poll = fs_revocable_poll;
+ if (fr->fops.unlocked_ioctl)
+ fr->fops.unlocked_ioctl = fs_revocable_unlocked_ioctl;
+
+ filp->f_op = &fr->fops;
+ filp->private_data = fr;
+ return 0;
+free_fr:
+ kfree(fr);
+ if (filp->f_op->release)
+ filp->f_op->release(filp->f_inode, filp);
+ return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(fs_revocable_replace);
diff --git a/include/linux/fs_revocable.h b/include/linux/fs_revocable.h
new file mode 100644
index 000000000000..498d035315e6
--- /dev/null
+++ b/include/linux/fs_revocable.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2025 Google LLC
+ */
+
+#ifndef __LINUX_FS_REVOCABLE_H
+#define __LINUX_FS_REVOCABLE_H
+
+#include <linux/fs.h>
+#include <linux/revocable.h>
+
+int fs_revocable_replace(struct revocable_provider *rp, struct file *filp);
+
+#endif /* __LINUX_FS_REVOCABLE_H */
--
2.48.1Hi Tzung-Bi,
kernel test robot noticed the following build errors:
[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on v6.18-rc4 next-20251106]
[cannot apply to char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus chrome-platform/for-next chrome-platform/for-firmware-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tzung-Bi-Shih/revocable-Add-fops-replacement/20251106-233108
base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link: https://lore.kernel.org/r/20251106152712.11850-2-tzungbi%40kernel.org
patch subject: [PATCH v6 1/3] revocable: Add fops replacement
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20251107/202511070909.JnNsfyvx-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511070909.JnNsfyvx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511070909.JnNsfyvx-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from fs/fs_revocable.c:9:
>> include/linux/fs_revocable.h:10:10: fatal error: 'linux/revocable.h' file not found
10 | #include <linux/revocable.h>
| ^~~~~~~~~~~~~~~~~~~
1 error generated.
vim +10 include/linux/fs_revocable.h
8
9 #include <linux/fs.h>
> 10 #include <linux/revocable.h>
11
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Tzung-Bi,
kernel test robot noticed the following build errors:
[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on v6.18-rc4 next-20251106]
[cannot apply to char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus chrome-platform/for-next chrome-platform/for-firmware-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tzung-Bi-Shih/revocable-Add-fops-replacement/20251106-233108
base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link: https://lore.kernel.org/r/20251106152712.11850-2-tzungbi%40kernel.org
patch subject: [PATCH v6 1/3] revocable: Add fops replacement
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20251107/202511070033.7X18bWdJ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511070033.7X18bWdJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511070033.7X18bWdJ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from fs/fs_revocable.c:9:
>> include/linux/fs_revocable.h:10:10: fatal error: linux/revocable.h: No such file or directory
10 | #include <linux/revocable.h>
| ^~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +10 include/linux/fs_revocable.h
8
9 #include <linux/fs.h>
> 10 #include <linux/revocable.h>
11
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> +/*
> + * Recover the private_data to its original one.
> + */
> +static struct fops_replacement *_recover_private_data(struct file *filp)
> +{
> + struct fops_replacement *fr = filp->private_data;
> +
> + filp->private_data = fr->orig_private_data;
> + return fr;
> +}
> +
> +/*
> + * Replace the private_data to fops_replacement.
> + */
> +static void _replace_private_data(struct fops_replacement *fr)
> +{
> + fr->filp->private_data = fr;
> +}
This switching of private_data isn't reasonable, it breaks too much
stuff. I think I showed a better idea in my sketch.
I still think this is a bad use case of revocable, we don't need to
obfuscate very simple locks in *core* kernel code like this. I'd rather
see you propose this series without using it.
> +static int fs_revocable_release(struct inode *inode, struct file *filp)
> +{
> + struct fops_replacement *fr = _recover_private_data(filp);
> + int ret = 0;
> + void *any;
> +
> + filp->f_op = fr->orig_fops;
> +
> + if (!fr->orig_fops->release)
> + goto leave;
> +
> + REVOCABLE_TRY_ACCESS_SCOPED(fr->rev, any) {
> + if (!any) {
> + ret = -ENODEV;
> + goto leave;
> + }
> +
> + ret = fr->orig_fops->release(inode, filp);
> + }
This probably doesn't work out, is likely to make a memory leak.
It will be hard for the owning driver to free its per-file memory
without access to release.
Jason
On Thu, Nov 06, 2025 at 11:47:15AM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> > +/*
> > + * Recover the private_data to its original one.
> > + */
> > +static struct fops_replacement *_recover_private_data(struct file *filp)
> > +{
> > + struct fops_replacement *fr = filp->private_data;
> > +
> > + filp->private_data = fr->orig_private_data;
> > + return fr;
> > +}
> > +
> > +/*
> > + * Replace the private_data to fops_replacement.
> > + */
> > +static void _replace_private_data(struct fops_replacement *fr)
> > +{
> > + fr->filp->private_data = fr;
> > +}
>
> This switching of private_data isn't reasonable, it breaks too much
> stuff. I think I showed a better idea in my sketch.
The approach assumes the filp->private_data should be set once by the
filp->f_op->open() if any. Is it common that the filp->private_data
be updated in other file operations?
> I still think this is a bad use case of revocable, we don't need to
> obfuscate very simple locks in *core* kernel code like this. I'd rather
> see you propose this series without using it.
>
> > +static int fs_revocable_release(struct inode *inode, struct file *filp)
> > +{
> > + struct fops_replacement *fr = _recover_private_data(filp);
> > + int ret = 0;
> > + void *any;
> > +
> > + filp->f_op = fr->orig_fops;
> > +
> > + if (!fr->orig_fops->release)
> > + goto leave;
> > +
> > + REVOCABLE_TRY_ACCESS_SCOPED(fr->rev, any) {
> > + if (!any) {
> > + ret = -ENODEV;
> > + goto leave;
> > + }
> > +
> > + ret = fr->orig_fops->release(inode, filp);
> > + }
>
> This probably doesn't work out, is likely to make a memory leak.
> It will be hard for the owning driver to free its per-file memory
> without access to release.
Ah, I think this reveals a drawback of the approach.
- Without calling ->release(), some memory may leak.
- With calling ->release(), some UAF may happen.
On Fri, Nov 07, 2025 at 05:07:54AM +0000, Tzung-Bi Shih wrote:
> On Thu, Nov 06, 2025 at 11:47:15AM -0400, Jason Gunthorpe wrote:
> > On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> > > +/*
> > > + * Recover the private_data to its original one.
> > > + */
> > > +static struct fops_replacement *_recover_private_data(struct file *filp)
> > > +{
> > > + struct fops_replacement *fr = filp->private_data;
> > > +
> > > + filp->private_data = fr->orig_private_data;
> > > + return fr;
> > > +}
> > > +
> > > +/*
> > > + * Replace the private_data to fops_replacement.
> > > + */
> > > +static void _replace_private_data(struct fops_replacement *fr)
> > > +{
> > > + fr->filp->private_data = fr;
> > > +}
> >
> > This switching of private_data isn't reasonable, it breaks too much
> > stuff. I think I showed a better idea in my sketch.
>
> The approach assumes the filp->private_data should be set once by the
> filp->f_op->open() if any. Is it common that the filp->private_data
> be updated in other file operations?
You can set it once during open, but you can't change it around every
fops callback. This stuff is all concurrent.
> > This probably doesn't work out, is likely to make a memory leak.
> > It will be hard for the owning driver to free its per-file memory
> > without access to release.
>
> Ah, I think this reveals a drawback of the approach.
> - Without calling ->release(), some memory may leak.
> - With calling ->release(), some UAF may happen.
It just means the user of this needs to understand there are
limitations on what release can do. Usually release just frees memory,
that is fine.
I think it would be strange for a release to touch revocable data,
that might suggest some larger problem.
Jason
On Fri, Nov 07, 2025 at 10:15:09AM -0400, Jason Gunthorpe wrote:
> On Fri, Nov 07, 2025 at 05:07:54AM +0000, Tzung-Bi Shih wrote:
> > On Thu, Nov 06, 2025 at 11:47:15AM -0400, Jason Gunthorpe wrote:
> > > On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> > > > +/*
> > > > + * Recover the private_data to its original one.
> > > > + */
> > > > +static struct fops_replacement *_recover_private_data(struct file *filp)
> > > > +{
> > > > + struct fops_replacement *fr = filp->private_data;
> > > > +
> > > > + filp->private_data = fr->orig_private_data;
> > > > + return fr;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Replace the private_data to fops_replacement.
> > > > + */
> > > > +static void _replace_private_data(struct fops_replacement *fr)
> > > > +{
> > > > + fr->filp->private_data = fr;
> > > > +}
> > >
> > > This switching of private_data isn't reasonable, it breaks too much
> > > stuff. I think I showed a better idea in my sketch.
> >
> > The approach assumes the filp->private_data should be set once by the
> > filp->f_op->open() if any. Is it common that the filp->private_data
> > be updated in other file operations?
>
> You can set it once during open, but you can't change it around every
> fops callback. This stuff is all concurrent.
Ah, yes, I see.
> > > This probably doesn't work out, is likely to make a memory leak.
> > > It will be hard for the owning driver to free its per-file memory
> > > without access to release.
> >
> > Ah, I think this reveals a drawback of the approach.
> > - Without calling ->release(), some memory may leak.
> > - With calling ->release(), some UAF may happen.
>
> It just means the user of this needs to understand there are
> limitations on what release can do. Usually release just frees memory,
> that is fine.
>
> I think it would be strange for a release to touch revocable data,
> that might suggest some larger problem.
I think it'd be inevitable for accessing some devm memory in ->release(),
e.g. [1].
[1] https://elixir.bootlin.com/linux/v6.17/source/drivers/platform/chrome/cros_ec_chardev.c#L260
On Mon, Nov 10, 2025 at 06:28:17AM +0000, Tzung-Bi Shih wrote: > > It just means the user of this needs to understand there are > > limitations on what release can do. Usually release just frees memory, > > that is fine. > > > > I think it would be strange for a release to touch revocable data, > > that might suggest some larger problem. > > I think it'd be inevitable for accessing some devm memory in ->release(), > e.g. [1]. > > [1] https://elixir.bootlin.com/linux/v6.17/source/drivers/platform/chrome/cros_ec_chardev.c#L260 Again, that's symptomatic of a "larger problem" :\ The blocking notifier registration to receive events to relay out to the file descriptors should be part this drivers probe/remove lifecycle so it has the proper lifetime. Trying to use a revocable idea here will create a weirdly unbalanced/dangling blocking notifier registration :( Inside cros_ec_chardev it should have a simple list of open file descriptors and the single blocking notifier callback can iterate over the list and memdup the events. Then your release is just a simple locked list del on a global list which doesn't have a lifecycle problem. Jason
© 2016 - 2025 Red Hat, Inc.