Re: [PATCH v4 5/7] revocable: Add fops replacement

[Tzung-Bi Shih]

On Wed, Sep 24, 2025 at 01:54:14PM -0700, dan.j.williams@xxxxxxxxx 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.