Re: [PATCH v5 5/7] revocable: Add fops replacement
From: Tzung-Bi Shih <tzungbi@kernel.org>
Date: 2025-10-17 02:37:03
Also in:
chrome-platform, linux-kselftest, lkml
On Thu, Oct 16, 2025 at 09:31:49AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 16, 2025 at 05:42:02AM +0000, Tzung-Bi Shih wrote:quoted
Introduce fs_revocable_replace() to simplify the use of the revocable API with file_operations. The function, should be called from a driver's ->open(), replaces the fops with a wrapper that automatically handles the `try_access` and `withdraw_access`. When the file is closed, the wrapper's ->release() restores the original fops and cleanups. This centralizes the revocable logic, making drivers cleaner and easier to maintain. Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> --- PoC patch. Known issues: - All file operations call revocable_try_access() for guaranteeing the resource even if the resource may be unused in the fops.Why is this so complicated?? You already added a per-flip struct:quoted
+struct fs_revocable_replacement { + const struct fs_revocable_operations *frops; + const struct file_operations *orig_fops; + struct file_operations fops; + struct revocable **revs; + size_t num_revs; +};Why does it need so much junk in it? struct fs_revocable_replacement { struct srcu_struct srcu; bool *alive; }; That's it. When the caller sets this up it provides a bool * pointer from it's own private struct that is kept krefcounted to life cycle of the struct file. Then the ops wrapers are a simple thing - generate them with a macro: srcu_read_lock(&f_rr->srcu); if (*f_rr_>alive) ret = f_rr->orig_fops->XX(...) else ret = -ENODEV; srcu_read_unlock(&f_rr->srcu); return ret; No need for all this revokable maze to do somethinig so simple.
Imagining the following example:
/* res1 and res2 are provided by hot-pluggable devices. */
struct filp_priv {
void *res1;
void *res2;
};
/* In .open() fops */
priv = kzalloc(sizeof(struct filp_priv), ...);
priv->res1 = ...;
priv->res2 = ...;
filp->private_data = priv;
/* In .read() fops */
priv = filp->private_data;
priv->res1 // could result UAF if the device has gone
priv->res2 // could result UAF if the device has gone
How does the bool * work for the example?