linux-kernel - Re: -next regression: "driver cohandle -EPROBE_DEFER from bus

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] [day] [month] [year] [list]

Date:	Fri, 18 Dec 2015 15:20:28 +0100
From:	Marek Szyprowski <m.szyprowski@...sung.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>,
	Dan Williams <dan.j.williams@...el.com>
Cc:	tomeu.vizoso@...labora.com,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-nvdimm@...ts.01.org" <linux-nvdimm@...1.01.org>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: -next regression:
 "driver cohandle -EPROBE_DEFER from bus_type.match()"

Hello,

On 2015-12-17 19:46, Russell King - ARM Linux wrote:
> On Thu, Dec 17, 2015 at 07:51:14AM -0800, Dan Williams wrote:
>> The commit below causes the libnvdimm sub-system to stop loading.
>> This is due to the fact that nvdimm_bus_match() returns the result of
>> test_bit() which may be negative.  If there are any other bus match
>> functions using test_bit they may be similarly impacted.
>>
>> Can we queue a fixup like the following to libnvdimm, and maybe
>> others, ahead of this driver core change?
> This is rather annoying.  Have we uncovered a latent bug in other
> architectures?  Well, looking through the test_bit() implementations,
> it looks like it.
>
> I'll drop the patch set for the time being, we can't go around breaking
> stuff like this.  However, I think the test_bit() result should be
> regularised across different architectures - it _looks_ to me like most
> implementations return 0/1 values, but there may be some that don't
> (maybe the assembly versions?)

I've checked all the functions assigned to match entry of struct bus_type
objects and found that there are only 4 such functions that don't return 0/1
values:

1. arch/arm/common/sa1111.c: sa1111_match -> result of bitwise &
2. drivers/nvdimm/bus.c: nvdimm_bus_match -> result of test_bit()
3. drivers/sh/superhyway/superhyway.c: superhyway_bus_match -> error codes
     (this is really funny case, all errors are resolved to 'matched' case)
4. drivers/staging/unisys/visorbus/visorbus_main.c: visorbus_match -> 0 and
     some positive integer values, safe for now

The list of functions that I've examined has been generated by following
command:
$ git grep -Pp "\.match\s" | grep -A1 bus_type | grep "\.match\s" | tr 
-s "\t;&,=:" " " | cut -d" " -f1,3

Only the first two functions require potential fixing to ensure that correct
match will not result in negative return value, so the following 
extension to
the patch proposed by Dan Williams fixes all of them:

From: Marek Szyprowski <m.szyprowski@...sung.com>
Date: Fri, 18 Dec 2015 15:17:12 +0100
Subject: [PATCH] drivers: ensure no negative value gets returned on positive
  match

Commit 09a14906a26e454cad7ff0ad96af40fc4cd90eb0 "ARM: 8472/1: driver
cohandle -EPROBE_DEFER from bus_type.match()") added support for returning
error values from bus_match callback. This patch ensures that all existing
match callbacks don't return negative values (potential errors) in case
of positive match.

Suggested-by: Dan Williams <dan.j.williams@...el.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@...sung.com>
---
  arch/arm/common/sa1111.c | 2 +-
  drivers/nvdimm/bus.c     | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index 3d22494..fb0a0a4 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -1290,7 +1290,7 @@ static int sa1111_match(struct device *_dev, 
struct device_driver *_drv)
         struct sa1111_dev *dev = SA1111_DEV(_dev);
         struct sa1111_driver *drv = SA1111_DRV(_drv);

-       return dev->devid & drv->devid;
+       return !!(dev->devid & drv->devid);
  }

  static int sa1111_bus_suspend(struct device *dev, pm_message_t state)
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..2b2181c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev, struct 
device_driver *drv)
  {
         struct nd_device_driver *nd_drv = to_nd_device_driver(drv);

-       return test_bit(to_nd_device_type(dev), &nd_drv->type);
+       return !!test_bit(to_nd_device_type(dev), &nd_drv->type);
  }

  static struct module *to_bus_provider(struct device *dev)
--
1.9.2


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/