linux-kernel - Re: [GIT PULL] Char/Misc driver patches for 4.12-rc1

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]

Date:   Fri, 5 May 2017 09:33:29 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     James Bottomley <James.Bottomley@...senpartnership.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Char/Misc driver patches for 4.12-rc1

On Fri, May 5, 2017 at 9:00 AM, James Bottomley
<James.Bottomley@...senpartnership.com> wrote:
>
> I'm not going to defend the earlier coding, but you've lost the real
> device_add() calls in the merge, meaning the tpm devices don't actually
> get made visible at all.  I suspect assuming device_add() is done by
> cdev_device_add() because of the name is going to be our next anti
> -pattern, so you're at least ahead of the game ...

Don't be silly. That's *exactly* what cdev_device_add() does.

That's the whole and only point of the whole function: it does *both*
the cdev_add() _and_ the device_add(), and it handles errors sanely
and unwinds things.

So removing the device_add() is what the code should do. It's also
exactly what the commit I pointed you at does (the one that clashed
with your addition of the new ":[c]devs" cases. Let me repeat:

  8dbbf5825181 tpm-chip: utilize new cdev_device_add helper function

So your patch looks like complete garbage, and I think you're confused
about what cdev_device_add() is.

I think you're confusing it with the nasty old "cdev_add()" function
that indeed didn't do a "device_add()", but that's exactly why
"cdev_device_add()" as added, so that you don't have to have that
nasty pattern of having both.

That said, I do think that my merge is missing a "cdev_device_del()"
in the [c]dev if the second cdev_device_add() (on [c]devs) fails.

Although I also considered just saying "maybe if the second device
register fails, we still want to just succeed, and at least leave the
first one available".  Because why shouldn't you let people access at
least the old interface even if the new interface couldn't be set up?

So that's part of why I'd like people to look at that resolutin. But
more importantly it should be tested.

               Linus