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, 05 May 2017 09:00:06 -0700
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     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 Thu, 2017-05-04 at 19:28 -0700, Linus Torvalds wrote:
> On Thu, May 4, 2017 at 5:18 PM, Greg KH <gregkh@...uxfoundation.org>
> wrote:
> > 
> > Here is the big set of new char/misc driver drivers and features 
> > for 4.12-rc1.
> 
> Ugh. I'm not particularly happy with the conflicts I got and my
> resolutions there-of.

Yes, we really should have done this via a postmerge tree.  We've had
so little cause to use them recently, I suspect everyone's forgotten
how.

> I did resolve them - in the case of drivers/scsi/osd/osd_uld.c as per
> James' suggestion from his SCSI pull. Thanks.
> 
> But even that resolution I'm not entirely happy with: somebody should
> check that it cleans up oud properly

It looks fine to me, but I'll ask the osd people (well person, since
it's very little used).

> But the one I'm really unhappy with is my tpm-chip.c resolution.
> 
> In particular, commit 8dbbf5825181 ("tpm-chip: utilize new
> cdev_device_add helper function") made the tpm-chip code use that
> cdev_device_add/del pattern for chip->[c]dev. Fine.
> 
> But then commit fdc915f7f719 ("tpm: expose spaces via a device link
> /dev/tpmrm<n>") added the *exact* same old pattern to a new
> "chip->[c]devs" (note the extra 's') and did so in a particularly
> ugly
> way too.
> 
> James, why did you do that nasty
> 
>         if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 
> *twice*, instead of just doing things properly inside *one* test?
> 
> Anyway, my merge resolution tries to just apply the same
> cdev_device_add/del pattern to the new chip->[c]devs entries, because
> not doing so seemed criminally ugly.
> 
> It compiles. It looks better than the mess it was. But it may not
> work.
> 
> James, Jarkko, you need to look at that tpm merge of mine. And James,
> double-check my osd_uld thing too.

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 ...

I think the below is the correct fix, but we should really take it
through the TPM list (or at least give me time for some runtime
testing) because I suddenly think the error leg unwinding isn't correct
in the original, so I've redone that too.

James

---

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 322b8a51ffc6..867cef5ca9e5 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -272,24 +272,30 @@ EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
 static int tpm_add_char_device(struct tpm_chip *chip)
 {
 	int rc;
+	const char *errstr;
+	struct device *errdev = &chip->dev;
 
 	rc = cdev_device_add(&chip->cdev, &chip->dev);
 	if (rc) {
-		dev_err(&chip->dev,
-			"unable to cdev_device_add() %s, major %d, minor %d, err=%d\n",
-			dev_name(&chip->dev), MAJOR(chip->dev.devt),
-			MINOR(chip->dev.devt), rc);
-		return rc;
+		errstr = "cdev_device_add for main device";
+		goto error1;
+	}
+	rc = device_add(&chip->dev);
+	if (rc) {
+		errstr = "device_add for main device";
+		goto error2;
 	}
-
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		errdev = &chip->devs;
 		rc = cdev_device_add(&chip->cdevs, &chip->devs);
 		if (rc) {
-			dev_err(&chip->devs,
-				"unable to cdev_device_add() %s, major %d, minor %d, err=%d\n",
-				dev_name(&chip->devs), MAJOR(chip->devs.devt),
-				MINOR(chip->devs.devt), rc);
-			return rc;
+			errstr = "cdev_device_add for rm device";
+			goto error3;
+		}
+		rc = device_add(&chip->devs);
+		if (rc) {
+			errstr = "device_add for rm device";
+			goto error4;
 		}
 	}
 
@@ -299,6 +305,19 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 	mutex_unlock(&idr_lock);
 
 	return rc;
+
+ error4:
+	cdev_del(&chip->cdevs);
+ error3:
+	device_del(&chip->dev);
+ error2:
+	cdev_del(&chip->cdev);
+ error1:
+	dev_err(&chip->dev,
+		"Failed %s %s, major %d, minor %d, err=%d\n",
+		errstr, dev_name(errdev), MAJOR(errdev->devt),
+		MINOR(errdev->devt), rc);
+	return rc;
 }
 
 static void tpm_del_char_device(struct tpm_chip *chip)