linux-kernel - Re: [PATCH v2 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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Dec 2020 15:06:37 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Christoph Hellwig <hch@....de>
Cc:     Joel Becker <jlbec@...lplan.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v2 3/4] configfs: implement committable items

On Tue, Dec 1, 2020 at 12:26 PM Christoph Hellwig <hch@....de> wrote:
>
> On Mon, Nov 30, 2020 at 05:47:03PM +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().
> >
> > 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.
> >
> > Implementation-wise: we reuse the default group mechanism to elegantly
> > plug the new pseude-groups into configfs. The pending group inherits the
> > parent group's operations so that config_items can be seamlesly created
> > in it using the callbacks supplied by the user as part of the committable
> > group itself.
>
> This looks pretty awkward in the hierachy, but I can't really think
> of anything else.  One idea would be to require fsync to stage updates,
> but that isn't really very well discoverable.

I'm not sure how that would work. fsync() a directory once the item is
configured to instantiate it?

I was thinking about different solutions other than the one already
defined but it always requires at least some kind of an additional
attribute (not defined by the user) to "commit" an item and in the end
rename() looks like a good candidate to me. We could possibly drop the
pending and live groups and simple use a magic suffix for committed
items e.g.: `mv myitem myitem_committed` but this would be even less
intuitive.

If this patch is extended with:

@@ -1103,6 +1103,8 @@ static void configfs_dump_one(struct
configfs_dirent *sd, int level)
  type_print(CONFIGFS_USET_DIR);
  type_print(CONFIGFS_USET_DEFAULT);
  type_print(CONFIGFS_USET_DROPPING);
+ type_print(CONFIGFS_GROUP_PENDING);
+ type_print(CONFIGFS_GROUP_LIVE);
 #undef type_print
 }

Then dumping the committable subsystem shows:

[  133.603035] configfs:    "04-committable-children":
[  133.603045] configfs:     CONFIGFS_DIR
[  133.603050] configfs:      "live":
[  133.603054] configfs:       CONFIGFS_DIR
[  133.603058] configfs:       CONFIGFS_USET_DIR
[  133.603062] configfs:       CONFIGFS_USET_DEFAULT
[  133.603066] configfs:       CONFIGFS_GROUP_LIVE
[  133.603070] configfs:        "dupa":
[  133.603074] configfs:         CONFIGFS_DIR
[  133.603078] configfs:          "committed":
[  133.603082] configfs:           CONFIGFS_ITEM_ATTR
[  133.603086] configfs:          "storeme":
[  133.603090] configfs:           CONFIGFS_ITEM_ATTR
[  133.603094] configfs:      "pending":
[  133.603160] configfs:       CONFIGFS_DIR
[  133.603167] configfs:       CONFIGFS_USET_DIR
[  133.603175] configfs:       CONFIGFS_USET_DEFAULT
[  133.603183] configfs:       CONFIGFS_GROUP_PENDING
[  133.603191] configfs:      "description":
[  133.603198] configfs:       CONFIGFS_ITEM_ATTR

Which doesn't look that bad IMO.

Bartosz