Re: [patch 103/147] lib/string: optimized memset

Re: [patch 103/147] lib/string: optimized memset

[Matteo Croce]

On Wed, 8 Sep 2021 11:34:27 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I'm dropping this one just to be consistent, although for memset()
> it's possibly a bit more reasonable to fall back on some default.
> 
> But probably not. memcpy and memset really are *so* special that these
> generic versions should be considered to be "stupid placeholders for
> bringup, and nothing more".
> 
> On Tue, Sep 7, 2021 at 7:58 PM Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > On a RISC-V machine the speed goes from 140 Mb/s to 241 Mb/s, and
> > this the binary size increase according to bloat-o-meter:
> 
> I also react to the benchmark numbers: RISC-V already has
> 
>   #define __HAVE_ARCH_MEMSET
>   #define __HAVE_ARCH_MEMCPY
>   #define __HAVE_ARCH_MEMMOVE
> 
> in its <asm/string.h> file, so these are just odd.
> 
> Did you benchmark these generic functions on their own, rather than
> the ones that actually get *used*?
> 
>            Linus

I benchmarked against the generic routines. The RISC-V specific are
even slower than the generic ones, because generates lot of unaligned
accesses.

That was the whole point of the series initially. These C routines
should have replaced the risc-v specific assembly ones, but then it was
proposed to use them as generic:

https://lore.kernel.org/linux-riscv/YNChl0tkofSGzvIX@infradead.org/

-- 
per aspera ad upstream

Re: [patch 136/147] nilfs2: use refcount_dec_and_lock() to fix potential UAF

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

Hi!

> From: Zhen Lei <thunder.leizhen@huawei.com>
> Subject: nilfs2: use refcount_dec_and_lock() to fix potential UAF
> 
> When the refcount is decreased to 0, the resource reclamation branch is
> entered.  Before CPU0 reaches the race point (1), CPU1 may obtain the
> spinlock and traverse the rbtree to find 'root', see nilfs_lookup_root(). 
> Although CPU1 will call refcount_inc() to increase the refcount, it is
> obviously too late.  CPU0 will release 'root' directly, CPU1 then accesses
> 'root' and triggers UAF.
> 
> Use refcount_dec_and_lock() to ensure that both the operations of decrease
> refcount to 0 and link deletion are lock protected eliminates this risk.
> 
>      CPU0                      CPU1
> nilfs_put_root():
> 			    <-------- (1)
> spin_lock(&nilfs->ns_cptree_lock);
> rb_erase(&root->rb_node, &nilfs->ns_cptree);
> spin_unlock(&nilfs->ns_cptree_lock);
> 
> kfree(root);
> 			    <-------- use-after-free

> There is no reproduction program, and the above is only theoretical
> analysis.

Ok, so we have a theoretical bug, and fix already on its way to
stable. But ... is it correct?

> +++ a/fs/nilfs2/the_nilfs.c
> @@ -792,14 +792,13 @@ nilfs_find_or_create_root(struct the_nil
>  
>  void nilfs_put_root(struct nilfs_root *root)
>  {
> -	if (refcount_dec_and_test(&root->count)) {
> -		struct the_nilfs *nilfs = root->nilfs;
> +	struct the_nilfs *nilfs = root->nilfs;
>  
> -		nilfs_sysfs_delete_snapshot_group(root);
> -
> -		spin_lock(&nilfs->ns_cptree_lock);
> +	if (refcount_dec_and_lock(&root->count, &nilfs->ns_cptree_lock)) {
>  		rb_erase(&root->rb_node, &nilfs->ns_cptree);
>  		spin_unlock(&nilfs->ns_cptree_lock);
> +
> +		nilfs_sysfs_delete_snapshot_group(root);
>  		iput(root->ifile);
>  
>  		kfree(root);

spin_lock() is deleted, but spin_unlock() is not affected. This means
unbalanced locking, right?

Best regards,
								Pavel
--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [patch 136/147] nilfs2: use refcount_dec_and_lock() to fix potential UAF

[Ryusuke Konishi]

Hi,

On Fri, Sep 24, 2021 at 7:35 PM Pavel Machek <pavel@denx.de> wrote:
>
> Hi!
>
> > From: Zhen Lei <thunder.leizhen@huawei.com>
> > Subject: nilfs2: use refcount_dec_and_lock() to fix potential UAF
> >
> > When the refcount is decreased to 0, the resource reclamation branch is
> > entered.  Before CPU0 reaches the race point (1), CPU1 may obtain the
> > spinlock and traverse the rbtree to find 'root', see nilfs_lookup_root().
> > Although CPU1 will call refcount_inc() to increase the refcount, it is
> > obviously too late.  CPU0 will release 'root' directly, CPU1 then accesses
> > 'root' and triggers UAF.
> >
> > Use refcount_dec_and_lock() to ensure that both the operations of decrease
> > refcount to 0 and link deletion are lock protected eliminates this risk.
> >
> >      CPU0                      CPU1
> > nilfs_put_root():
> >                           <-------- (1)
> > spin_lock(&nilfs->ns_cptree_lock);
> > rb_erase(&root->rb_node, &nilfs->ns_cptree);
> > spin_unlock(&nilfs->ns_cptree_lock);
> >
> > kfree(root);
> >                           <-------- use-after-free
>
> > There is no reproduction program, and the above is only theoretical
> > analysis.
>
> Ok, so we have a theoretical bug, and fix already on its way to
> stable. But ... is it correct?
>
> > +++ a/fs/nilfs2/the_nilfs.c
> > @@ -792,14 +792,13 @@ nilfs_find_or_create_root(struct the_nil
> >
> >  void nilfs_put_root(struct nilfs_root *root)
> >  {
> > -     if (refcount_dec_and_test(&root->count)) {
> > -             struct the_nilfs *nilfs = root->nilfs;
> > +     struct the_nilfs *nilfs = root->nilfs;
> >
> > -             nilfs_sysfs_delete_snapshot_group(root);
> > -
> > -             spin_lock(&nilfs->ns_cptree_lock);
> > +     if (refcount_dec_and_lock(&root->count, &nilfs->ns_cptree_lock)) {
> >               rb_erase(&root->rb_node, &nilfs->ns_cptree);
> >               spin_unlock(&nilfs->ns_cptree_lock);
> > +
> > +             nilfs_sysfs_delete_snapshot_group(root);
> >               iput(root->ifile);
> >
> >               kfree(root);
>
> spin_lock() is deleted, but spin_unlock() is not affected. This means
> unbalanced locking, right?

It's okay.   spin_lock() is integrated into refcount_dec_and_lock(), which was
originally refcount_dec_and_test().

Thanks,
Ryusuke Konishi

>
> Best regards,
>                                                                 Pavel
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>