Re: [PATCH v2] io_uring/register: add IORING_REGISTER_CLONE_FILES opcode

On 6/17/26 18:33, Gabriel Krisman Bertazi wrote:
> IIUC, the IORING_REGISTER_DST_REPLACE exists for backward compatibility,
> since originally the buffer cloning would fail if existing elements were
> already there.  It is kind of superflous in a new operation but I suppose
> it is here to mirror the semantics of io_clone_buffers, which is ok, but
> then...
>
> This free should at least be gated on ctx->file_table->data.nr.  We are
> always replacing the ->file_table if it was initialized, so it is a bit
> more logical to check the table directly.

Agreed. In v3, I have gated the io_free_file_tables() call directly on 
ctx->file_table.data.nr as suggested.

> > +	/* not allowed unless REPLACE is set */
> > +	if (!(clone_arg.flags & IORING_REGISTER_DST_REPLACE) &&
> > +	    ctx->file_table.data.nr)
> > +		return -EBUSY;
> This check is duplicated in io_clone_files.

I have removed this duplicate check from the parent function in v3.

> > +	src_ctx = file->private_data;
> > +	if (src_ctx != ctx) {
> Shouldn't we just fail if ctx == src_ctx ?

Yes, I have updated this to explicitly fail with -EINVAL 
if ctx == src_ctx to prevent self-cloning in v3. Thanks for catching this.

> > +		/* Prevent cross-process hijacking */
> > +		if (src_ctx->submitter_task &&
> > +		    src_ctx->submitter_task != current) {
> > +			ret = -EEXIST;
> > +			goto out;
> Is limiting the feature to the submitter_task necessary to safely copy
> the table even if the lock is held?  The use-case for this feature would
> be setting up a single ring with its file table and then replicating it
> on other threads, on the common model of one-ring per thread. This check
> limits it.

I have entirely removed the submitter_task check in v3.