linux-kernel - Re: [PATCH 3/4] configfs: implement committable items

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 14 Jan 2021 15:19:17 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     Christoph Hellwig <hch@....de>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] configfs: implement committable items

On Thu, Jan 14, 2021 at 9:40 AM Bartosz Golaszewski
<bgolaszewski@...libre.com> wrote:
>
> On Thu, Jan 14, 2021 at 12:46 AM Joel Becker <jlbec@...lplan.org> wrote:
> >
> > On Wed, Nov 25, 2020 at 04:22:46PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > >
> > > This implements configfs committable items. We mostly follow the
> > > documentation except that we extend config_group_ops with uncommit_item()
> > > callback for reverting the changes made by commit_item().
> >
> > Woohoo!  A long time coming, but thank you for working on the
> > implementation!
> >
> > > Each committable group has two sub-directories: pending and live. New
> > > items can only be created in pending/. Attributes can only be modified
> > > while the item is in pending/. Once it's ready to be committed, it must
> > > be moved over to live/ using the rename() system call. This is when the
> > > commit_item() function will be called.
> >
> > The original API intended for live items to still be modifyable.  The
> > live/ path forbids mkdir()/rmdir(), but it allows store().  Otherwise,
> > items can't be adjusted at all while in use, which is severely limiting.
> > Obviously the store() handler must not allow transitions from
> > valid-value->invalid-value, but the handler would reject invalid values
> > anyway, wouldn't it?
> >
> > > diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> > > index 1f0270229d7b..a20e55fd05e8 100644
> > > --- a/fs/configfs/file.c
> > > +++ b/fs/configfs/file.c
> > > @@ -243,9 +243,17 @@ fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, size
> > >  static int
> > >  flush_write_buffer(struct file *file, struct configfs_buffer *buffer, size_t count)
> > >  {
> > > +     struct config_item *parent_item = buffer->item->ci_parent;
> > >       struct configfs_fragment *frag = to_frag(file);
> > > +     struct configfs_dirent *sd;
> > >       int res = -ENOENT;
> > >
> > > +     if (parent_item && parent_item->ci_dentry) {
> > > +             sd = parent_item->ci_dentry->d_fsdata;
> > > +             if (sd->s_type & CONFIGFS_GROUP_LIVE)
> > > +                     return -EPERM;
> > > +     }
> > > +
> > >       down_read(&frag->frag_sem);
> > >       if (!frag->frag_dead)
> > >               res = buffer->attr->store(buffer->item, buffer->page, count);
> >
> > Basically, I would just leave this hunk out.
> >
>
> I would make this configurable per-attribute because for the use-case
> I need this for we definitely don't want the items to be modifiable
> once they're "live".
>
> Bartosz

After a second thought: I agree this can be left to the user's
discretion so I'll just remove this and enforce it in the sample code.

One thing I'm unsure about is whether we should allow to change the
group's name when moving it from pending to live. Any thoughts?

Bartosz