linux-kernel - Re: What to do on cdev

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: <20160714064735.GA14568@kroah.com>
Date:	Thu, 14 Jul 2016 15:47:35 +0900
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Jean Delvare <jdelvare@...e.de>
Cc:	linux-fsdevel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: What to do on cdev_add failure

On Wed, Jul 13, 2016 at 03:46:14PM +0200, Jean Delvare wrote:
> Hi all,
> 
> I am currently working on the i2c-dev driver, which has just been
> converted to the non-ancestral cdev API. As I am cleaning up the
> driver, I would like to switch from static cdev initialization
> (cdev_init) to dynamic allocation (cdev_alloc.)
> 
> While I was looking at other drivers to figure out how to deal with
> error cases, I found that different drivers do different things if
> cdev_add fails after cdev_alloc was called successfully. I guess some
> of them are right, others are wrong, and I'd like to know which is
> which ;-)
> 
> * char/virtio_console.c, s390/char/tape_class.c, s390/char/vmur.c,
>   infiniband/.../qib_file_ops.c, fuse/cuse.c, scsi/sg.c and scsi/st.g
>   are calling cdev_del(cdev).
> * v4l2-core/v4l2-dev.c is calling kfree(cdev).
> * s390/char/vmlogrdr.c, uio/uio.c, tty/ty_io.c and __register_chrdev()
>   are calling kobject_put(&cdev->kobj). The former explicitly says "no
>   cdev_del here!" in a comment.
> 
> My gut feeling is that kobject_put(&cdev->kobj) is correct, even though
> it feels strange to have to use a low-level function to clean-up after
> a higher level API call.
> 
> If cdev_del(cdev) is also correct (and as I read the code it could be,
> iff calling kobj_unmap() is a no-op if kobj_map() failed - is it the
> case?), then it should be clearly documented as such, as it is
> counter-intuitive (to me, at least.)
> 
> Anyone wants to comment on this?
> 
> On top of this, another thing looks strange to me. cdev_add() only gets
> the parent kobj on success. However the release methods
> (cdev_default_release and cdev_dynamic_release) will put the parent
> kobj unconditionally. So it looks to me that we are over-putting the
> parent whenever cdev_add() fails. OTOH I can't see where the parent is
> set. If it is NULL then all these get and put are no-ops to start with
> and it no longer matters. But why would we be doing that?
> 
> Then again, what do I know about kobj black magic...

It's worse than you think, the kobject in a cdev is not a "real"
kobject.  Well, it's kind of real, but it's only there to be used for
the kmap logic.  I have a 10+ year old TODO item here that says "remove
kobj from cdev" that I really should get to one of these days.

Anyone that touches the kobj outside of the cdev core code is probably
wrong, it's "funny" that both uio and tty do that, the maintainer of
that code must be lazy... :)

Let me look into what the "correct" thing to do here is, I used to know
it, need some time to refresh my memory...

And the cdev interface has what, 4 different ways it can be used?
Another of my TODO items is to fix it all up to only use it one way, or
maybe just 2 as it does have the ability to make driver code pretty
small if you use it in unique ways...

thanks,

greg k-h