Re: [PATCH v2 6/6] efivarfs: fix error on write to new variable leaving remnants

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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