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.
