linux-kernel - Re: [PATCH v2 2/3] fsnotify: Protect i_fsnotify

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]

Message-ID: <CAOQ4uxh7OvmH6o1fUmMoQ_D347jVBx53TLe4R=BjtXTuvCzKCA@mail.gmail.com>
Date:   Fri, 21 Oct 2022 12:18:28 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Stephen Brennan <stephen.s.brennan@...cle.com>
Cc:     Jan Kara <jack@...e.cz>, Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] fsnotify: Protect i_fsnotify_mask and child flags
 with inode rwsem

On Fri, Oct 21, 2022 at 11:22 AM Amir Goldstein <amir73il@...il.com> wrote:
...
> > +/*
> > + * Objects may need some additional actions to be taken when the last reference
> > + * is dropped. Define flags to indicate which actions are necessary.
> > + */
> > +#define FSNOTIFY_OBJ_FLAG_NEED_IPUT            0x01
> > +#define FSNOTIFY_OBJ_FLAG_UPDATE_CHILDREN      0x02
>
> with changed_flags argument, you do not need these, you can use
> the existing CONN_FLAGS.
>
> It is a bit ugly that the direction of the change is not expressed
> in changed_flags, but for the current code, it is not needed, because
> update_children does care about the direction of the change and
> the direction of change to HAS_IREF is expressed by the inode
> object return value.
>

Oh that is a lie...

return value can be non NULL because of an added mark
that wants iref and also wants to watch children, but the
only practical consequence of this is that you can only
do the WARN_ON for the else case of update_children
in fsnotify_recalc_mask().

I still think it is a win for code simplicity as I detailed
in my comments.

> Maybe try it out in v3 to see how it works.
>
> Unless Jan has an idea that will be easier to read and maintain...
>

Maybe fsnotify_update_inode_conn_flags() should return "update_flags"
and not "changed_flags", because actually the WATCHING_CHILDREN
flag is not changed by the helper itself.

Then, HAS_IREF is not returned when helper did get_iref() and changed
HAS_IREF itself and then the comment that says:
     /* Unpin inode after detach of last mark that wanted iref */
will be even clearer:

        if (want_iref) {
                /* Pin inode if any mark wants inode refcount held */
                fsnotify_get_inode_ref(fsnotify_conn_inode(conn));
                conn->flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
        } else {
                /* Unpin inode after detach of last mark that wanted iref */
                ret = inode;
                update_flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
        }

Thanks,
Amir.