[PATCH v2 0/3] dell-wmi: DMI misuse fixes

[PATCH v2 0/3] dell-wmi: DMI misuse fixes

[Andy Lutomirski]

Darren, patches 1 and 2 are for platform-drivers-x86, assuming that
people like them when reviewed.  They're probably 4.5 material, since
they fix a real bug, even if it's hard to trigger.

Jean, patch 3 is for you if you like it.

Changes from v1:
 - Patch 1 has minor tweaks from Jean.
 - Patches 2 and 3 are new.

Andy Lutomirski (3):
  dell-wmi: Stop storing pointers to DMI tables
  dell-wmi: Fix hotkey table size check
  dmi: Make dmi_walk and dmi_walk_early return real error codes

 drivers/firmware/dmi_scan.c     |  6 ++--
 drivers/platform/x86/dell-wmi.c | 71 +++++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 31 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

[Andy Lutomirski]

The dmi_walk function maps the DMI table, walks it, and unmaps it.
This means that the dell_bios_hotkey_table that find_hk_type stores
points to unmapped memory by the time it gets read.

I've been able to trigger crashes caused by the stale pointer a
couple of times, but never on a stock kernel.

Fix it by generating the keymap in the dmi_walk callback instead of
storing a pointer.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Notes:
    Changes from v1:
     - Rename handle_dmi_table to handle_dmi_entry (Jean)
     - Remove useless assignment to results->err (Jean)

 drivers/platform/x86/dell-wmi.c | 68 +++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index b6f193b07566..5c0d037fcd40 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -114,7 +114,10 @@ struct dell_bios_hotkey_table {
 
 };
 
-static const struct dell_bios_hotkey_table *dell_bios_hotkey_table;
+struct dell_dmi_results {
+	int err;
+	struct key_entry *keymap;
+};
 
 /* Uninitialized entries here are KEY_RESERVED == 0. */
 static const u16 bios_to_linux_keycode[256] __initconst = {
@@ -315,20 +318,34 @@ static void dell_wmi_notify(u32 value, void *context)
 	kfree(obj);
 }
 
-static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
+static void __init handle_dmi_entry(const struct dmi_header *dm,
+				    void *opaque)
 {
-	int hotkey_num = (dell_bios_hotkey_table->header.length - 4) /
-				sizeof(struct dell_bios_keymap_entry);
+	struct dell_dmi_results *results = opaque;
+	struct dell_bios_hotkey_table *table;
 	struct key_entry *keymap;
-	int i;
+	int hotkey_num, i;
+
+	if (results->err || results->keymap)
+		return;		/* We already found the hotkey table. */
+
+	if (dm->type != 0xb2 || dm->length <= 6)
+		return;
+
+	table = container_of(dm, struct dell_bios_hotkey_table, header);
+
+	hotkey_num = (table->header.length - 4) /
+				sizeof(struct dell_bios_keymap_entry);
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
-	if (!keymap)
-		return NULL;
+	if (!keymap) {
+		results->err = -ENOMEM;
+		return;
+	}
 
 	for (i = 0; i < hotkey_num; i++) {
 		const struct dell_bios_keymap_entry *bios_entry =
-					&dell_bios_hotkey_table->keymap[i];
+					&table->keymap[i];
 
 		/* Uninitialized entries are 0 aka KEY_RESERVED. */
 		u16 keycode = (bios_entry->keycode <
@@ -357,11 +374,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
 
 	keymap[hotkey_num].type = KE_END;
 
-	return keymap;
+	results->keymap = keymap;
 }
 
 static int __init dell_wmi_input_setup(void)
 {
+	struct dell_dmi_results dmi_results = {};
 	int err;
 
 	dell_wmi_input_dev = input_allocate_device();
@@ -372,20 +390,26 @@ static int __init dell_wmi_input_setup(void)
 	dell_wmi_input_dev->phys = "wmi/input0";
 	dell_wmi_input_dev->id.bustype = BUS_HOST;
 
-	if (dell_new_hk_type) {
-		const struct key_entry *keymap = dell_wmi_prepare_new_keymap();
-		if (!keymap) {
-			err = -ENOMEM;
-			goto err_free_dev;
-		}
+	err = dmi_walk(handle_dmi_entry, &dmi_results);
+	if (err)
+		goto err_free_dev;
 
-		err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL);
+	if (dmi_results.err) {
+		err = dmi_results.err;
+		goto err_free_dev;
+	}
+
+	if (dmi_results.keymap) {
+		dell_new_hk_type = true;
+
+		err = sparse_keymap_setup(dell_wmi_input_dev,
+					  dmi_results.keymap, NULL);
 
 		/*
 		 * Sparse keymap library makes a copy of keymap so we
 		 * don't need the original one that was allocated.
 		 */
-		kfree(keymap);
+		kfree(dmi_results.keymap);
 	} else {
 		err = sparse_keymap_setup(dell_wmi_input_dev,
 					  dell_wmi_legacy_keymap, NULL);
@@ -412,15 +436,6 @@ static void dell_wmi_input_destroy(void)
 	input_unregister_device(dell_wmi_input_dev);
 }
 
-static void __init find_hk_type(const struct dmi_header *dm, void *dummy)
-{
-	if (dm->type == 0xb2 && dm->length > 6) {
-		dell_new_hk_type = true;
-		dell_bios_hotkey_table =
-			container_of(dm, struct dell_bios_hotkey_table, header);
-	}
-}
-
 static int __init dell_wmi_init(void)
 {
 	int err;
@@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
-	dmi_walk(find_hk_type, NULL);
 
 	err = dell_wmi_input_setup();
 	if (err)
-- 
2.5.0

Re: [PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

[Jean Delvare]

Hi Andy,

On Mon, 18 Jan 2016 12:59:38 -0800, Andy Lutomirski wrote:
> The dmi_walk function maps the DMI table, walks it, and unmaps it.
> This means that the dell_bios_hotkey_table that find_hk_type stores
> points to unmapped memory by the time it gets read.
> 
> I've been able to trigger crashes caused by the stale pointer a
> couple of times, but never on a stock kernel.
> 
> Fix it by generating the keymap in the dmi_walk callback instead of
> storing a pointer.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Notes:
>     Changes from v1:
>      - Rename handle_dmi_table to handle_dmi_entry (Jean)
>      - Remove useless assignment to results->err (Jean)
> 
>  drivers/platform/x86/dell-wmi.c | 68 +++++++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 27 deletions(-)
> (...)
> @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
>  		return -ENODEV;
>  	}
>  
> -	dmi_walk(find_hk_type, NULL);
>  

Please also delete the following blank line, to avoid two consecutive
blank lines.

>  	err = dell_wmi_input_setup();
>  	if (err)

Other than this, it looks good to me, so after fixing the above, you
can add:

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

[Jean Delvare]

Hi Andy,

One more thing I just noticed...

On Mon, 18 Jan 2016 12:59:38 -0800, Andy Lutomirski wrote:
> @@ -372,20 +390,26 @@ static int __init dell_wmi_input_setup(void)
>  	dell_wmi_input_dev->phys = "wmi/input0";
>  	dell_wmi_input_dev->id.bustype = BUS_HOST;
>  
> -	if (dell_new_hk_type) {
> -		const struct key_entry *keymap = dell_wmi_prepare_new_keymap();
> -		if (!keymap) {
> -			err = -ENOMEM;
> -			goto err_free_dev;
> -		}
> +	err = dmi_walk(handle_dmi_entry, &dmi_results);
> +	if (err)
> +		goto err_free_dev;
> (...)
> @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
>  		return -ENODEV;
>  	}
>  
> -	dmi_walk(find_hk_type, NULL);
>  
>  	err = dell_wmi_input_setup();
>  	if (err)

Before, in the absence of DMI support, the driver would load and assume
the old hotkey interface. After your change, the driver will fail to
load if DMI support is missing. This could, in theory, cause a
regression (although I would be very surprised if even the oldest
supported models don't implement DMI.)

If you want this patch to make it into stable, you probably should
stick to the original behavior and move that change (if you still
believe it is a good idea) to a separate patch, and at the same time
make DELL_WMI either select or depend on DMI. Many other platform/x86
drivers should do the same, BTW, but that's a separate topic.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

[Andy Lutomirski]

On Tue, Jan 19, 2016 at 3:04 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Andy,
>
> One more thing I just noticed...
>
> On Mon, 18 Jan 2016 12:59:38 -0800, Andy Lutomirski wrote:
>> @@ -372,20 +390,26 @@ static int __init dell_wmi_input_setup(void)
>>       dell_wmi_input_dev->phys = "wmi/input0";
>>       dell_wmi_input_dev->id.bustype = BUS_HOST;
>>
>> -     if (dell_new_hk_type) {
>> -             const struct key_entry *keymap = dell_wmi_prepare_new_keymap();
>> -             if (!keymap) {
>> -                     err = -ENOMEM;
>> -                     goto err_free_dev;
>> -             }
>> +     err = dmi_walk(handle_dmi_entry, &dmi_results);
>> +     if (err)
>> +             goto err_free_dev;
>> (...)
>> @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
>>               return -ENODEV;
>>       }
>>
>> -     dmi_walk(find_hk_type, NULL);
>>
>>       err = dell_wmi_input_setup();
>>       if (err)
>
> Before, in the absence of DMI support, the driver would load and assume
> the old hotkey interface. After your change, the driver will fail to
> load if DMI support is missing. This could, in theory, cause a
> regression (although I would be very surprised if even the oldest
> supported models don't implement DMI.)

I switched it from a failure to a pr_warn.

>
> If you want this patch to make it into stable, you probably should
> stick to the original behavior and move that change (if you still
> believe it is a good idea) to a separate patch, and at the same time
> make DELL_WMI either select or depend on DMI. Many other platform/x86
> drivers should do the same, BTW, but that's a separate topic.
>

I'm going to add a patch for that to the series, without Cc: stable.
I don't think it's a big enough deal to be worth -stable churn.

--Andy

[PATCH v2 2/3] dell-wmi: Fix hotkey table size check

[Andy Lutomirski]

The minimum size of the table is 4, not 6.  Replace the hard-coded
number with a sizeof expression.  While we're at it, repace the
hard-coded 4 below as well.

Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/platform/x86/dell-wmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 5c0d037fcd40..48838942d593 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
 struct dell_bios_hotkey_table {
 	struct dmi_header header;
 	struct dell_bios_keymap_entry keymap[];
-
 };
 
 struct dell_dmi_results {
@@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
 
-	if (dm->type != 0xb2 || dm->length <= 6)
+	if (dm->type != 0xb2 ||
+	    dm->length <= sizeof(struct dell_bios_hotkey_table))
 		return;
 
 	table = container_of(dm, struct dell_bios_hotkey_table, header);
 
-	hotkey_num = (table->header.length - 4) /
+	hotkey_num = (table->header.length -
+		      sizeof(struct dell_bios_hotkey_table)) /
 				sizeof(struct dell_bios_keymap_entry);
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
-- 
2.5.0

Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check

[Jean Delvare]

Hi Andy,

On Mon, 18 Jan 2016 12:59:39 -0800, Andy Lutomirski wrote:
> The minimum size of the table is 4, not 6.  Replace the hard-coded
> number with a sizeof expression.  While we're at it, repace the
> hard-coded 4 below as well.
> 
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  drivers/platform/x86/dell-wmi.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 5c0d037fcd40..48838942d593 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
>  struct dell_bios_hotkey_table {
>  	struct dmi_header header;
>  	struct dell_bios_keymap_entry keymap[];
> -
>  };
>  
>  struct dell_dmi_results {

Nice cleanup but in general we recommend to not mix style cleanups with
functional changes. If you want to clean up dell-wmi you could do it in
a separate patch and maybe include the fixes suggested by checkpatch.pl
-f.

> @@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>  	if (results->err || results->keymap)
>  		return;		/* We already found the hotkey table. */
>  
> -	if (dm->type != 0xb2 || dm->length <= 6)
> +	if (dm->type != 0xb2 ||
> +	    dm->length <= sizeof(struct dell_bios_hotkey_table))
>  		return;

I'm confused. sizeof(struct dell_bios_hotkey_table) is 4. Given that
dm->length is guaranteed to be at least 4 per the SMBIOS specification,
you are really only testing that dm->length != 4. Which means you are
still accepting 5, 6 and 7, even though they would lead to hotkey_num =
0 below.

If the purpose of this check is only to guarantee that the container_of
below is valid then you should check for dm->length < sizeof(struct
dell_bios_hotkey_table) (not <=.) This is still useless in practice but
I can understand and accept it because it is conceptually correct.

OTOH if the purpose of the check is to ensure that there is at least
one hotkey, you should check for dm->length < sizeof(struct
dell_bios_hotkey_table) + sizeof(struct dell_bios_keymap_entry)
instead. hotkey_num could also be checked separately below but it is
more efficient to have a single test.

>  
>  	table = container_of(dm, struct dell_bios_hotkey_table, header);
>  
> -	hotkey_num = (table->header.length - 4) /
> +	hotkey_num = (table->header.length -
> +		      sizeof(struct dell_bios_hotkey_table)) /
>  				sizeof(struct dell_bios_keymap_entry);
>  
>  	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check

[Andy Lutomirski]

On Tue, Jan 19, 2016 at 12:31 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Andy,
>
> On Mon, 18 Jan 2016 12:59:39 -0800, Andy Lutomirski wrote:
>> The minimum size of the table is 4, not 6.  Replace the hard-coded
>> number with a sizeof expression.  While we're at it, repace the
>> hard-coded 4 below as well.
>>
>> Reported-by: Jean Delvare <jdelvare@suse.de>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  drivers/platform/x86/dell-wmi.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index 5c0d037fcd40..48838942d593 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
>>  struct dell_bios_hotkey_table {
>>       struct dmi_header header;
>>       struct dell_bios_keymap_entry keymap[];
>> -
>>  };
>>
>>  struct dell_dmi_results {
>
> Nice cleanup but in general we recommend to not mix style cleanups with
> functional changes. If you want to clean up dell-wmi you could do it in
> a separate patch and maybe include the fixes suggested by checkpatch.pl
> -f.

/me sheepishly puts the newline back in.

>
>> @@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>>       if (results->err || results->keymap)
>>               return;         /* We already found the hotkey table. */
>>
>> -     if (dm->type != 0xb2 || dm->length <= 6)
>> +     if (dm->type != 0xb2 ||
>> +         dm->length <= sizeof(struct dell_bios_hotkey_table))
>>               return;
>
> I'm confused. sizeof(struct dell_bios_hotkey_table) is 4. Given that
> dm->length is guaranteed to be at least 4 per the SMBIOS specification,
> you are really only testing that dm->length != 4. Which means you are
> still accepting 5, 6 and 7, even though they would lead to hotkey_num =
> 0 below.
>
> If the purpose of this check is only to guarantee that the container_of
> below is valid then you should check for dm->length < sizeof(struct
> dell_bios_hotkey_table) (not <=.) This is still useless in practice but
> I can understand and accept it because it is conceptually correct.
>
> OTOH if the purpose of the check is to ensure that there is at least
> one hotkey, you should check for dm->length < sizeof(struct
> dell_bios_hotkey_table) + sizeof(struct dell_bios_keymap_entry)
> instead. hotkey_num could also be checked separately below but it is
> more efficient to have a single test.

I think the check is just to see if the buffer is big enough, but
maybe there's history here, and I don't want to be the old to break
ancient laptops for the sake of a cleanup.  Let me try this again.

--Andy

[PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Andy Lutomirski]

Currently they return -1 on error, which will confuse callers if
they try to interpret it as a normal negative error code.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/firmware/dmi_scan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 0e08e665f715..451ac04ed18d 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 
 	buf = dmi_early_remap(dmi_base, orig_dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, NULL);
 
@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
 
 	buf = dmi_remap(dmi_base, dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, private_data);
 
-- 
2.5.0

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Jean Delvare]

Hi Andy,

On Mon, 18 Jan 2016 12:59:40 -0800, Andy Lutomirski wrote:
> Currently they return -1 on error, which will confuse callers if
> they try to interpret it as a normal negative error code.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  drivers/firmware/dmi_scan.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 0e08e665f715..451ac04ed18d 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  
>  	buf = dmi_early_remap(dmi_base, orig_dmi_len);
>  	if (buf == NULL)
> -		return -1;
> +		return -ENOMEM;
>  
>  	dmi_decode_table(buf, decode, NULL);
>  
> @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>  	u8 *buf;
>  
>  	if (!dmi_available)
> -		return -1;
> +		return -ENOENT;

-ENOSYS would seem more appropriate?

>  
>  	buf = dmi_remap(dmi_base, dmi_len);
>  	if (buf == NULL)
> -		return -1;
> +		return -ENOMEM;
>  
>  	dmi_decode_table(buf, decode, private_data);
>  

There is a formatted comment before dmi_walk, which says "Returns -1
when the DMI table can't be reached". This comment needs to be updated.

Please also update the dmi_walk stub in include/linux/dmi.h when
CONFIG_DMI isn't set.

I don't think this patch should be in this series, I'd rather take it
in my dmi tree and push it to Linus immediately.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Pali Rohár]

On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
> > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> >  	u8 *buf;
> >  
> >  	if (!dmi_available)
> > -		return -1;
> > +		return -ENOENT;
> 
> -ENOSYS would seem more appropriate?

IIRC -ENOSYS is for non implemented syscalls.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Jean Delvare]

Hi Pali,

On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote:
> On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
> > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> > >  	u8 *buf;
> > >  
> > >  	if (!dmi_available)
> > > -		return -1;
> > > +		return -ENOENT;
> > 
> > -ENOSYS would seem more appropriate?
> 
> IIRC -ENOSYS is for non implemented syscalls.

I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when
a specific subsystem is not included. Not related to syscalls at all.
This is what lead to my suggestion.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Pali Rohár]

On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:
> Hi Pali,
> 
> On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote:
> > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
> > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> > > >  	u8 *buf;
> > > >  
> > > >  	if (!dmi_available)
> > > > -		return -1;
> > > > +		return -ENOENT;
> > > 
> > > -ENOSYS would seem more appropriate?
> > 
> > IIRC -ENOSYS is for non implemented syscalls.
> 
> I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when
> a specific subsystem is not included. Not related to syscalls at all.
> This is what lead to my suggestion.

https://lkml.org/lkml/2014/8/22/492

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Jean Delvare]

On Tue, 19 Jan 2016 10:07:36 +0100, Pali Rohár wrote:
> On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:
> > Hi Pali,
> > 
> > On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote:
> > > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
> > > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> > > > >  	u8 *buf;
> > > > >  
> > > > >  	if (!dmi_available)
> > > > > -		return -1;
> > > > > +		return -ENOENT;
> > > > 
> > > > -ENOSYS would seem more appropriate?
> > > 
> > > IIRC -ENOSYS is for non implemented syscalls.
> > 
> > I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when
> > a specific subsystem is not included. Not related to syscalls at all.
> > This is what lead to my suggestion.
> 
> https://lkml.org/lkml/2014/8/22/492

Thanks for the pointer, I wasn't aware of that.

It really should be documented. No, checkpatch.pl isn't documentation.

Also the commit sadly doesn't say why using ENOSYS in other contexts is
considered a bad thing. What actual trouble did it cause?

Are the current presumably incorrect uses of ENOSYS ultimately going to
be fixed? If not, I see no point in preventing other use cases.

Also what about errno(3)? It says ENOSYS is "Function not implemented"
- no mention to syscalls. And glibc agrees (set errno to ENOSYS and
call perror, it says "Function not implemented.") In contradiction with
the proposed rule.

Back to the patch under review, I would argue that ENOENT is solely for
file-related operations (after all the man page says "No such file or
directory"), which is why I did not like it. Then what is left to mean
"function not available"? ENOTSUP?

Whatever the answer is, it should also be documented and added to the
checkpatch warning message (if we stick to that plan...) If you tell
people "don't use this" without telling them what to use instead, each
of us will come up with something different and this will lead to
inconsistency. Which is worse than everybody using the same (maybe
suboptimal) error code.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Andy Lutomirski]

On Tue, Jan 19, 2016 at 1:40 AM, Jean Delvare <jdelvare@suse.de> wrote:
> On Tue, 19 Jan 2016 10:07:36 +0100, Pali Rohár wrote:
>> On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:
>> > Hi Pali,
>> >
>> > On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote:
>> > > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
>> > > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>> > > > >       u8 *buf;
>> > > > >
>> > > > >       if (!dmi_available)
>> > > > > -             return -1;
>> > > > > +             return -ENOENT;
>> > > >
>> > > > -ENOSYS would seem more appropriate?
>> > >
>> > > IIRC -ENOSYS is for non implemented syscalls.
>> >
>> > I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when
>> > a specific subsystem is not included. Not related to syscalls at all.
>> > This is what lead to my suggestion.
>>
>> https://lkml.org/lkml/2014/8/22/492
>
> Thanks for the pointer, I wasn't aware of that.
>
> It really should be documented. No, checkpatch.pl isn't documentation.
>
> Also the commit sadly doesn't say why using ENOSYS in other contexts is
> considered a bad thing. What actual trouble did it cause?

The trouble is that user code likes to assume that, when a syscall
returns -ENOSYS, that syscall isn't implemented.  Letting ENOSYS leak
out to userspace via a syscall that *is* implemented can confused
things.

>
> Are the current presumably incorrect uses of ENOSYS ultimately going to
> be fixed? If not, I see no point in preventing other use cases.

We at least want to prevent it from newly introduced syscalls.

I'll try to clean up the docs.

--Andy