On Thu, 2025-01-16 at 18:59 +0000, Al Viro wrote:
> On Thu, Jan 16, 2025 at 01:54:44PM -0500, James Bottomley wrote:
> > On Thu, 2025-01-16 at 18:45 +0000, Al Viro wrote:
> > > On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote:
> > >
> > > > + inode_lock(inode);
> > > > + if (d_unhashed(file->f_path.dentry)) {
> > > > + /*
> > > > + * file got removed; don't allow a set. Caused by an
> > > > + * unsuccessful create or successful delete write
> > > > + * racing with us.
> > > > + */
> > > > + bytes = -EIO;
> > > > + goto out;
> > > > + }
> > >
> > > Wouldn't the check for zero ->i_size work here? Would be easier
> > > to follow...
> >
> > Unfortunately not. The pathway for creating a variable involves a
> > call to efivarfs_create() (create inode op) first, which would in
> > itself create a zero length file, then a call to
> > efivarfs_file_write(), so if we key here on zero length we'd never
> > be able to create new variables.
> >
> > The idea behind the check is that delete could race with write and
> > if so, we can't resurrect the variable once it's been unhashed from
> > the directory, so we need to error out at that point.
>
> D'oh... Point, but it still feels as if you are misplacing the
> object state here ;-/
>
> OK, so we have
> * created, open but yet to be written into
> * live
> * removed
>
> Might be better off with explicit state in efivar_entry...
OK, that would get rid of the race in efivarfs_file_release I'd been
worrying about where we can decide to remove the file under the inode
lock but have to make it unhashed after dropping the inode lock.
Regards,
James
