linux-kernel - Re: [PATCH v2 1/2] vfs: add rcu-based find

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: <CAGudoHE12-7c0kmVpKz8HyBeHt8jX8hOQ7zQxZNJ0Re7FF8r6g@mail.gmail.com>
Date: Tue, 11 Jun 2024 07:17:41 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: brauner@...nel.org, viro@...iv.linux.org.uk, jack@...e.cz, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-btrfs@...r.kernel.org, josef@...icpanda.com
Subject: Re: [PATCH v2 1/2] vfs: add rcu-based find_inode variants for iget ops

On Tue, Jun 11, 2024 at 6:59 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> > +EXPORT_SYMBOL(iget5_locked_rcu);
>
> EXPORT_SYMBOL_GPL for rcu APIs.
>

noted for v3, thanks

> > +static void __wait_on_freeing_inode(struct inode *inode, bool locked)
> >  {
> >       wait_queue_head_t *wq;
> >       DEFINE_WAIT_BIT(wait, &inode->i_state, __I_NEW);
> >       wq = bit_waitqueue(&inode->i_state, __I_NEW);
> >       prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
> >       spin_unlock(&inode->i_lock);
> > -     spin_unlock(&inode_hash_lock);
> > +     rcu_read_unlock();
> > +     if (locked)
> > +             spin_unlock(&inode_hash_lock);
>
> The conditional locking here is goign to make sparse rather unhappy.
> Please try to find a way to at least annotate it, or maybe find
> another way around like, like leaving the schedule in finish_wait
> in the callers.
>

So I tried out sparse on my patch vs fs-next and found it emits the
same warnings.

fs/inode.c:846:17: warning: context imbalance in 'inode_lru_isolate' -
unexpected unlock
fs/inode.c:901:9: warning: context imbalance in 'find_inode' -
different lock contexts for basic block
fs/inode.c:932:9: warning: context imbalance in 'find_inode_fast' -
different lock contexts for basic block
fs/inode.c:1621:5: warning: context imbalance in 'insert_inode_locked'
- wrong count at exit
fs/inode.c:1739:20: warning: context imbalance in 'iput_final' -
unexpected unlock
fs/inode.c:1753:6: warning: context imbalance in 'iput' - wrong count at exit
fs/inode.c:2238:13: warning: context imbalance in
'__wait_on_freeing_inode' - unexpected unlock

The patch does not make things *worse*, so I don't think messing with
the code is warranted here.

> > +extern struct inode *ilookup5_nowait_rcu(struct super_block *sb,
> > +             unsigned long hashval, int (*test)(struct inode *, void *),
> > +             void *data);
>
> No need for the extern here (or down below).
>

I agree, but this is me just copying and modifying an existing line.

include/linux/fs.h is chock full of extern-prefixed func declarations,
on top of that some name the arguments while the rest does not.

Someone(tm) should definitely clean it up, but I'm not interested in
bikeshedding about it.

-- 
Mateusz Guzik <mjguzik gmail.com>