linux-kernel - Re: [PATCH 05/11] fs: add iterate_supers_excl() and iterate_supers_reverse

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:   Thu, 30 Nov 2017 02:40:38 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>, bart.vanassche@....com,
        ming.lei@...hat.com, "Ted Ts'o" <tytso@....edu>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        Jiri Kosina <jikos@...nel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        linux-fsdevel@...r.kernel.org,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Todd Brandt <todd.e.brandt@...ux.intel.com>, nborisov@...e.com,
        Jan Kara <jack@...e.cz>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Oliver Neukum <ONeukum@...e.com>, oleksandr@...alenko.name,
        Oleg Antonyan <oleg.b.antonyan@...il.com>,
        Yu Chen <yu.chen.surf@...il.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        linux-block@...r.kernel.org, linux-xfs@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 05/11] fs: add iterate_supers_excl() and iterate_supers_reverse_excl()

On Thu, Nov 30, 2017 at 2:34 AM, Dave Chinner <david@...morbit.com> wrote:
> On Thu, Nov 30, 2017 at 12:48:15AM +0100, Rafael J. Wysocki wrote:
>> On Thu, Nov 30, 2017 at 12:23 AM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
>> > There are use cases where we wish to traverse the superblock list
>> > but also capture errors, and in which case we want to avoid having
>> > our callers issue a lock themselves since we can do the locking for
>> > the callers. Provide a iterate_supers_excl() which calls a function
>> > with the write lock held. If an error occurs we capture it and
>> > propagate it.
>> >
>> > Likewise there are use cases where we wish to traverse the superblock
>> > list but in reverse order. The new iterate_supers_reverse_excl() helpers
>> > does this but also also captures any errors encountered.
>> >
>> > Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
>> > ---
>> >  fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/fs.h |  2 ++
>> >  2 files changed, 93 insertions(+)
>> >
>> > diff --git a/fs/super.c b/fs/super.c
>> > index a63513d187e8..885711c1d35b 100644
>> > --- a/fs/super.c
>> > +++ b/fs/super.c
>> > @@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
>> >         spin_unlock(&sb_lock);
>> >  }
>> >
>> > +/**
>> > + *     iterate_supers_excl - exclusively call func for all active superblocks
>> > + *     @f: function to call
>> > + *     @arg: argument to pass to it
>> > + *
>> > + *     Scans the superblock list and calls given function, passing it
>> > + *     locked superblock and given argument. Returns 0 unless an error
>> > + *     occurred on calling the function on any superblock.
>> > + */
>> > +int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
>> > +{
>> > +       struct super_block *sb, *p = NULL;
>> > +       int error = 0;
>> > +
>> > +       spin_lock(&sb_lock);
>> > +       list_for_each_entry(sb, &super_blocks, s_list) {
>> > +               if (hlist_unhashed(&sb->s_instances))
>> > +                       continue;
>> > +               sb->s_count++;
>> > +               spin_unlock(&sb_lock);
>>
>> Can anything bad happen if the list is modified at this point by a
>> concurrent thread?
>
> No. We have a valid reference to sb->s_count and that keeps it on
> the list while we have the lock dropped. The sb reference isn't
> dropped until we've iterated to the next sb on the list and taken a
> reference to that, hence it's safe to drop and regain the list lock
> without needing to restart the iteration.
>
>> > +
>> > +               down_write(&sb->s_umount);
>> > +               if (sb->s_root && (sb->s_flags & SB_BORN)) {
>> > +                       error = f(sb, arg);
>> > +                       if (error) {
>> > +                               up_write(&sb->s_umount);
>> > +                               spin_lock(&sb_lock);
>> > +                               __put_super(sb);
>> > +                               break;
>> > +                       }
>> > +               }
>> > +               up_write(&sb->s_umount);
>> > +
>> > +               spin_lock(&sb_lock);
>> > +               if (p)
>> > +                       __put_super(p);
>> > +               p = sb;
>
> This code here is what drops the reference to the previous sb
> we've iterated past.
>
> FWIW, this "hold until next is held" iteration pattern is used
> frequently for inodes, dentries, and other reference counted VFS
> objects so we can iterate the list without needing to hold the
> list lock for the entire iteration....

OK, thanks!