On Wed, Sep 24, 2025 at 01:54:14PM -0700, dan.j.williams@intel.com wrote:
Tzung-Bi Shih wrote:
+int revocable_replace_fops(struct file *filp, struct revocable_provider *rp,
const struct revocable_operations *rops)+{
- struct fops_replacement *fr;
- fr = kzalloc(sizeof(*fr), GFP_KERNEL);
- if (!fr)
return -ENOMEM;- fr->filp = filp;
- fr->rops = rops;
- fr->orig_fops = filp->f_op;
- fr->rev = revocable_alloc(rp);
- if (!fr->rev)
return -ENOMEM;- memcpy(&fr->fops, filp->f_op, sizeof(struct file_operations));
- scoped_guard(mutex, &fops_replacement_mutex)
list_add(&fr->list, &fops_replacement_list);This list grows for every active instance? Unless I am misreading, that looks like a scaling burden that the simple approach below does not have.
Correct, unless we want to embed the context (e.g. struct fops_replacement) into struct file. FWIW: the issue also listed as a known issue after "---".
- fr->fops.release = revocable_fr_release;
- if (filp->f_op->read)
fr->fops.read = revocable_fr_read;- if (filp->f_op->poll)
fr->fops.poll = revocable_fr_poll;- if (filp->f_op->unlocked_ioctl)
fr->fops.unlocked_ioctl = revocable_fr_unlocked_ioctl;- filp->f_op = &fr->fops;
- return 0;
+}
This facility is protecting the wrong resource, and I argue hides bugs in drivers that think they need this. That matches the conclusion I came to with my "managed_fops" attempt.
The resource that is being revoked is the device's attachment to its driver. Whether that is dev_get_drvdata() or some other device-to-data lookup, that is the resource that gets removed, not the fops themselves. The only resource race with fops is whether the code text section remains available while the fops are registered, but that lifetime scope is not at a per-device instance scope.
revocable_replace_fops() doesn't protect any resources. It replaces the fops to revocable wrappers and recovers the fops when the file is releasing.