Re: [RFC v2 08/16] luo: luo_files: add infrastructure for FDs

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On Thu, May 15, 2025 at 7:16 PM James Houghton <jthoughton@google.com> wrote:
>
> On Thu, May 15, 2025 at 11:23 AM Pasha Tatashin
> <pasha.tatashin@soleen.com> wrote:
> > +/**
> > + * luo_retrieve_file - Find a registered file instance by its token.
> > + * @token: The unique token of the file instance to retrieve.
> > + * @file: Output parameter. On success (return value 0), this will point
> > + * to the retrieved "struct file".
> > + *
> > + * Searches the global list for a &struct luo_file matching the @token. Uses a
> > + * read lock, allowing concurrent retrievals.
> > + *
> > + * Return: 0 on success. Negative errno on failure.
> > + */
> > +int luo_retrieve_file(u64 token, struct file **file)
> > +{
> > +       struct luo_file *luo_file;
> > +       int ret = 0;
> > +
> > +       luo_files_recreate_luo_files_xa_in();
> > +       luo_state_read_enter();
> > +       if (!liveupdate_state_updated()) {
> > +               pr_warn("File can be retrieved only in updated state\n");
> > +               luo_state_read_exit();
> > +               return -EBUSY;
> > +       }
> > +
> > +       luo_file = xa_load(&luo_files_xa_in, token);
> > +       if (luo_file && !luo_file->reclaimed) {
> > +               luo_file->reclaimed = true;
>
> I haven't been able to pay too much attention to the series yet, and I
> know this was posted as an RFC, so pardon my nit-picking.
>
> I think you need to have xchg here for this not to be racy, so something like:
>
> `if (luo_file && !xchg(&luo_file->reclaimed, true))`
>
> Or maybe you meant to avoid this race some other way; IIUC,
> luo_state_read_enter() is not sufficient.

Thank you for catching this. This is a bug, I actually added a per fd
mutex lock to struct luo_file that is supposed to be used here. I am
going to address this in the next version.

Thanks,
Pasha

>
> Thanks!
>
> > +               ret = luo_file->fs->retrieve(luo_file->fs->arg,
> > +                                            luo_file->private_data,
> > +                                            file);
> > +               if (!ret)
> > +                       luo_file->file = *file;
> > +       } else if (luo_file && luo_file->reclaimed) {
> > +               pr_err("The file descriptor for token %lld has already been retrieved\n",
> > +                      token);
> > +               ret = -EINVAL;
> > +       } else {
> > +               ret = -ENOENT;
> > +       }
> > +
> > +       luo_state_read_exit();
> > +
> > +       return ret;
> > +}