linux-kernel - Re: kobject_init_and

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200604002316.GM6578@ziepe.ca>
Date:   Wed, 3 Jun 2020 21:23:16 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     James Bottomley <James.Bottomley@...senPartnership.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Wang Hai <wanghai38@...wei.com>, cl@...ux.com,
        penberg@...nel.org, rientjes@...gle.com, iamjoonsoo.kim@....com,
        akpm@...ux-foundation.org, khlebnikov@...dex-team.ru,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: kobject_init_and_add is easy to misuse

On Wed, Jun 03, 2020 at 01:56:20PM -0700, James Bottomley wrote:

> The store of state_in_sysfs is already done in kobject_add_internal(). 
> It's an existing flag people already use to tell if the kobject has
> been exposed in sysfs.  However, it's set after the sysfs directory
> creation succeeds.  This is the code with some debugging removed:
> 
> 	error = create_dir(kobj);
> 	if (error) {
> 		kobj_kset_leave(kobj);
> 		kobject_put(parent);
> 		kobj->parent = NULL;
> 		...
> 	} else
> 		kobj->state_in_sysfs = 1;

I was thinking most probably this will need a lock or a
smp_store_release() ..

> > It feels more robust to stick with the put though..
> 
> possibly ... like I said, the only concern with the put path is that
> ->release has state expectations that aren't met if
> kobject_init_and_add fails.

Certainly error unwind bugs related to put and release will exist, but
I suspect switching to kfree won't solve them, just move them to the
next function that fails and needs a put based unwind?

At least the patches I reviewed for RDM a from Wang Hai were all
correct and didn't seem to have release based errors.

Jason