Re: [PATCH] mac80211_hwsim: release driver when ieee80211_register_hw fails

[Martin Pitt]

Acked-By: Martin Pitt 

Hello Junjie,

Junjie Mao [2014-10-28  9:31 +0800]:
> The driver is not released when ieee80211_register_hw fails in
> mac80211_hwsim_create_radio, leading to the access to the unregistered (and
> possibly freed) device in platform_driver_unregister:

Many thanks for fixing this! Sorry about that, I don't know these bits
very well.

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [PATCH] mac80211_hwsim: release driver when ieee80211_register_hw fails

[Martin Pitt]

Acked-By: Martin Pitt martin.p...@ubuntu.com

Hello Junjie,

Junjie Mao [2014-10-28  9:31 +0800]:
 The driver is not released when ieee80211_register_hw fails in
 mac80211_hwsim_create_radio, leading to the access to the unregistered (and
 possibly freed) device in platform_driver_unregister:

Many thanks for fixing this! Sorry about that, I don't know these bits
very well.

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [mac80211_hwsim] BUG: unable to handle kernel paging request at ce1db404

[Martin Pitt]

Fengguang Wu [2014-03-08 20:11 +0800]:
> [4.429993] mac80211_hwsim: ieee80211_register_hw failed (-2)
> [...]
> [4.431924]  [] get_device+0xf/0x17
> [4.431924]  [] driver_detach+0x38/0x8f
> [4.431924]  [] bus_remove_driver+0x53/0x66
> [4.431924]  [] driver_unregister+0x38/0x3d
> [4.431924]  [] platform_driver_unregister+0xb/0xd
> [4.431924]  [] init_mac80211_hwsim+0x3a5/0x3b6


So that first message is from
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/mac80211_hwsim.c?id=9ea927748#n2428

At this point we registered the platform driver and the class, and it
should have created two devices (at least for the default radios=2).
What's odd is that I don't see this printk in your kernel log:

  mac80211_hwsim: Initializing radio %d

If for some reasons "radio" is 0, it would not show this and not
initialize data->dev, but then you shouldn't get to
ieee80211_register_hw() either as it's in the same loop. So that's a
bit of a mystery to me.

On failure, above ieee80211_register_hw() jumps to the cleanup:

| failed_hw:
|   device_unregister(data->dev);
| failed_drvdata:
|   ieee80211_free_hw(hw);
| failed:
|   mac80211_hwsim_free();
| failed_unregister_driver:
|   driver_unregister(_hwsim_driver);
|   return err;
| }


The mac80211_hwsim_free() function again calls
device_unregister(data->dev) for a list (not sure which, I'm not
certain how to interpret

  list_for_each_entry_safe(data, tmpdata, , list)

) Could that be the double free causing the memory corruption?

If you are in a position to do quick builds and tests, does the crash
go away with this?

printk(KERN_DEBUG "mac80211_hwsim: device_bind_driver failed 
(%d)\n",
   err);
-   goto failed_hw;
+   goto failed_drvdata;
}

(I'm not claiming that this is correct, just taking a stab at
understanding what happens) If not, does it go away with changing the
goto to failed_unregister_driver()?

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [mac80211_hwsim] BUG: unable to handle kernel paging request at ce1db404

[Martin Pitt]

Hey Fengguang,

Fengguang Wu [2014-03-05 21:23 +0800]:
> git bisect start v3.10 v3.9 --
> git bisect  bad ff89acc563a0bd49965674f56552ad6620415fe2  # 03:01  0- 
>  2  Merge branch 'rcu/urgent' of 
> git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu
> git bisect  bad 24d0c2542b38963ae4d5171ecc0a2c1326c656bc  # 03:03  0- 
>  2  Merge branch 'for-linus' of 
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
> git bisect good 151173e8ce9b95d7eedb9035cfaffbdb7cb2  # 03:11 20+ 
> 20  Merge tag 'for-v3.10' of git://git.infradead.org/battery-2.6
> git bisect  bad e95893004104054d49406fd108fefa3ddc054366  # 03:13  0- 
>  4  Merge tag 'for_linus' of 
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
> git bisect good 8a72f3820c4d14b27ad5336aed00063a7a7f1bef  # 03:19 20+ 
> 20  Merge git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile
> git bisect  bad 600fe9751aeb6f6b72de84076a05c5b8c04152c0  # 03:21  0- 
>  5  ipc_schedule_free() can do vfree() directly now
> git bisect  bad 126de6b20bfb82cc19012d5048f11f339ae5a021  # 03:23  0- 
>  1  linkage.h: fix build breakage due to symbol prefix handling
> git bisect good 251df49db3327c64bf917bfdba94491fde2b4ee0  # 03:27 20+ 
> 20  Merge branch 'for-linus' of 
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
> git bisect  bad 73287a43cc79ca06629a88d1a199cd283f42456a  # 03:29  0- 
>  2  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
> git bisect good 953c96e0d85615d1ab1f100e525d376053294dc2  # 03:37 20+ 
>  0  tg3: Use bool not int
> git bisect  bad 4fc4118cdb29ab946b8a586fc766ebb6ae1e1c90  # 03:39  0- 
>  2  wil6210: more Rx descriptor accessor functions
> git bisect good e73dcfbf061b524fe9aaef56cf3c2e234a45ec19  # 03:46 20+ 
>  7  Bluetooth: hidp: fix sending output reports on intr channel
> git bisect good d5590bba37f3c7d496195648532d5313abb43891  # 03:50 20+ 
>  0  NFC: pn533: Re-group fields in struct pn533
> git bisect  bad 06d961a8e210035bff7e82f466107f9ab4a8fd94  # 03:52  0- 
>  7  mac80211/minstrel: use the new rate control API
> git bisect  bad 97990a060e6757f48b931a3946b17c1c4362c3fb  # 03:54  0- 
>  3  nl80211: allow using wdev identifiers to get scan results
> git bisect  bad 85220d71bf3ca1ba9129e0744247ae5f61bec559  # 03:56  1- 
>  5  mac80211: support secondary channel offset in CSA
> git bisect  bad 0ca54f6c5fd4ce58aa044d1fc7f00d7f6cf2801c  # 03:57  1- 
>  6  mac80211: provide SSID in IBSS mode
> git bisect  bad 3088f7d2db42925808c4b43a6258647ee4d1dd5f  # 03:59  3- 
>  8  mac80211: stringify another plink state
> git bisect  bad 9d6d6f4924133567a108a862d9cf949cd03f71cb  # 04:02  0- 
>  2  mac80211: unset FC retry bit in mesh fwding path
> git bisect  bad 9ea927748ced4953f1e9a0f1fa1fdeacd1018b4e  # 04:05  0- 
>  9  mac80211_hwsim: Register and bind to driver
> # first bad commit: [9ea927748ced4953f1e9a0f1fa1fdeacd1018b4e] 
> mac80211_hwsim: Register and bind to driver
> git bisect good ddc4db2e3d5393ede7a9222bb3b7522a603a4678  # 04:27 69+ 
> 18  mac80211: make ieee802_11_parse_elems an inline
> git bisect  bad b148a42ba7823e34971cd4e5b05a5c74fa3311ed  # 04:27  0- 
> 19  Add linux-next specific files for 20140228
> git bisect  bad d8efcf38b13df3e9e889cf7cc214cb85dc53600c  # 04:30  0- 
>  3  Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
> git bisect  bad b148a42ba7823e34971cd4e5b05a5c74fa3311ed  # 04:30  0- 
> 19  Add linux-next specific files for 20140228

I noticed that this bisect list didn't include Sasha Levin's followup fix:

  
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/wireless/mac80211_hwsim.c?id=c07fe5ae06

That was supposed to fix problems at registration. Is that actually
inclued in your bisection? I suppose it is, but it's quite clear that
with only 9ea927 you'd get crashes with building it statically.

I have no real idea why ieee80211_register_hw() would fail (but then
again, I'm a kernel n00b). It's apparently related to doing
platform_driver_register() before, so perhaps it's missing another
field in the struct. "unable to handle kernel paging request", is that
a normal error message if a driver fails to initialize? I would have
assumed that the kernel would't panic if a single driver fails to
initialize. Or does that mean it actually tries to access uninit'ed
memory?

I can certainly check out linux-next, build the whole thing
statically, and try to twiddle things a bit, but I'm afraid that's
just going to take a while (both because this is all new to me and I'm
working on other things ATM), so please don't hold your breath. :-)

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [mac80211_hwsim] BUG: unable to handle kernel paging request at ce1db404

[Martin Pitt]

Hey Fengguang,

Fengguang Wu [2014-03-05 21:23 +0800]:
 git bisect start v3.10 v3.9 --
 git bisect  bad ff89acc563a0bd49965674f56552ad6620415fe2  # 03:01  0- 
  2  Merge branch 'rcu/urgent' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu
 git bisect  bad 24d0c2542b38963ae4d5171ecc0a2c1326c656bc  # 03:03  0- 
  2  Merge branch 'for-linus' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
 git bisect good 151173e8ce9b95d7eedb9035cfaffbdb7cb2  # 03:11 20+ 
 20  Merge tag 'for-v3.10' of git://git.infradead.org/battery-2.6
 git bisect  bad e95893004104054d49406fd108fefa3ddc054366  # 03:13  0- 
  4  Merge tag 'for_linus' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
 git bisect good 8a72f3820c4d14b27ad5336aed00063a7a7f1bef  # 03:19 20+ 
 20  Merge git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile
 git bisect  bad 600fe9751aeb6f6b72de84076a05c5b8c04152c0  # 03:21  0- 
  5  ipc_schedule_free() can do vfree() directly now
 git bisect  bad 126de6b20bfb82cc19012d5048f11f339ae5a021  # 03:23  0- 
  1  linkage.h: fix build breakage due to symbol prefix handling
 git bisect good 251df49db3327c64bf917bfdba94491fde2b4ee0  # 03:27 20+ 
 20  Merge branch 'for-linus' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
 git bisect  bad 73287a43cc79ca06629a88d1a199cd283f42456a  # 03:29  0- 
  2  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
 git bisect good 953c96e0d85615d1ab1f100e525d376053294dc2  # 03:37 20+ 
  0  tg3: Use bool not int
 git bisect  bad 4fc4118cdb29ab946b8a586fc766ebb6ae1e1c90  # 03:39  0- 
  2  wil6210: more Rx descriptor accessor functions
 git bisect good e73dcfbf061b524fe9aaef56cf3c2e234a45ec19  # 03:46 20+ 
  7  Bluetooth: hidp: fix sending output reports on intr channel
 git bisect good d5590bba37f3c7d496195648532d5313abb43891  # 03:50 20+ 
  0  NFC: pn533: Re-group fields in struct pn533
 git bisect  bad 06d961a8e210035bff7e82f466107f9ab4a8fd94  # 03:52  0- 
  7  mac80211/minstrel: use the new rate control API
 git bisect  bad 97990a060e6757f48b931a3946b17c1c4362c3fb  # 03:54  0- 
  3  nl80211: allow using wdev identifiers to get scan results
 git bisect  bad 85220d71bf3ca1ba9129e0744247ae5f61bec559  # 03:56  1- 
  5  mac80211: support secondary channel offset in CSA
 git bisect  bad 0ca54f6c5fd4ce58aa044d1fc7f00d7f6cf2801c  # 03:57  1- 
  6  mac80211: provide SSID in IBSS mode
 git bisect  bad 3088f7d2db42925808c4b43a6258647ee4d1dd5f  # 03:59  3- 
  8  mac80211: stringify another plink state
 git bisect  bad 9d6d6f4924133567a108a862d9cf949cd03f71cb  # 04:02  0- 
  2  mac80211: unset FC retry bit in mesh fwding path
 git bisect  bad 9ea927748ced4953f1e9a0f1fa1fdeacd1018b4e  # 04:05  0- 
  9  mac80211_hwsim: Register and bind to driver
 # first bad commit: [9ea927748ced4953f1e9a0f1fa1fdeacd1018b4e] 
 mac80211_hwsim: Register and bind to driver
 git bisect good ddc4db2e3d5393ede7a9222bb3b7522a603a4678  # 04:27 69+ 
 18  mac80211: make ieee802_11_parse_elems an inline
 git bisect  bad b148a42ba7823e34971cd4e5b05a5c74fa3311ed  # 04:27  0- 
 19  Add linux-next specific files for 20140228
 git bisect  bad d8efcf38b13df3e9e889cf7cc214cb85dc53600c  # 04:30  0- 
  3  Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
 git bisect  bad b148a42ba7823e34971cd4e5b05a5c74fa3311ed  # 04:30  0- 
 19  Add linux-next specific files for 20140228

I noticed that this bisect list didn't include Sasha Levin's followup fix:

  
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/wireless/mac80211_hwsim.c?id=c07fe5ae06

That was supposed to fix problems at registration. Is that actually
inclued in your bisection? I suppose it is, but it's quite clear that
with only 9ea927 you'd get crashes with building it statically.

I have no real idea why ieee80211_register_hw() would fail (but then
again, I'm a kernel n00b). It's apparently related to doing
platform_driver_register() before, so perhaps it's missing another
field in the struct. unable to handle kernel paging request, is that
a normal error message if a driver fails to initialize? I would have
assumed that the kernel would't panic if a single driver fails to
initialize. Or does that mean it actually tries to access uninit'ed
memory?

I can certainly check out linux-next, build the whole thing
statically, and try to twiddle things a bit, but I'm afraid that's
just going to take a while (both because this is all new to me and I'm
working on other things ATM), so please don't hold your breath. :-)

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [mac80211_hwsim] BUG: unable to handle kernel paging request at ce1db404

[Martin Pitt]

Fengguang Wu [2014-03-08 20:11 +0800]:
 [4.429993] mac80211_hwsim: ieee80211_register_hw failed (-2)
 [...]
 [4.431924]  [c12377de] get_device+0xf/0x17
 [4.431924]  [c123a165] driver_detach+0x38/0x8f
 [4.431924]  [c1239433] bus_remove_driver+0x53/0x66
 [4.431924]  [c123a535] driver_unregister+0x38/0x3d
 [4.431924]  [c123b3aa] platform_driver_unregister+0xb/0xd
 [4.431924]  [c1c4ac9f] init_mac80211_hwsim+0x3a5/0x3b6


So that first message is from
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/mac80211_hwsim.c?id=9ea927748#n2428

At this point we registered the platform driver and the class, and it
should have created two devices (at least for the default radios=2).
What's odd is that I don't see this printk in your kernel log:

  mac80211_hwsim: Initializing radio %d

If for some reasons radio is 0, it would not show this and not
initialize data-dev, but then you shouldn't get to
ieee80211_register_hw() either as it's in the same loop. So that's a
bit of a mystery to me.

On failure, above ieee80211_register_hw() jumps to the cleanup:

| failed_hw:
|   device_unregister(data-dev);
| failed_drvdata:
|   ieee80211_free_hw(hw);
| failed:
|   mac80211_hwsim_free();
| failed_unregister_driver:
|   driver_unregister(mac80211_hwsim_driver);
|   return err;
| }


The mac80211_hwsim_free() function again calls
device_unregister(data-dev) for a list (not sure which, I'm not
certain how to interpret

  list_for_each_entry_safe(data, tmpdata, tmplist, list)

) Could that be the double free causing the memory corruption?

If you are in a position to do quick builds and tests, does the crash
go away with this?

printk(KERN_DEBUG mac80211_hwsim: device_bind_driver failed 
(%d)\n,
   err);
-   goto failed_hw;
+   goto failed_drvdata;
}

(I'm not claiming that this is correct, just taking a stab at
understanding what happens) If not, does it go away with changing the
goto to failed_unregister_driver()?

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [PATCH] mac80211_hwsim: correctly register the platform driver

[Martin Pitt]

Hello Sasha,

Sasha Levin [2013-04-24  0:40 -0400]:
> Not registering a platform_driver would make us access garbage
> when the platform callbacks under driver_register() kicks in.
> 
> Signed-off-by: Sasha Levin 

Ah, thanks for catching this! I applied that patch and verified that
it still works well.

Tested-By: Martin Pitt 

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Re: [PATCH] mac80211_hwsim: correctly register the platform driver

[Martin Pitt]

Hello Sasha,

Sasha Levin [2013-04-24  0:40 -0400]:
 Not registering a platform_driver would make us access garbage
 when the platform callbacks under driver_register() kicks in.
 
 Signed-off-by: Sasha Levin sasha.le...@oracle.com

Ah, thanks for catching this! I applied that patch and verified that
it still works well.

Tested-By: Martin Pitt martin.p...@ubuntu.com

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

mac80211_hwsim: Check return value of driver_register

[Martin Pitt]

If driver_register() fails, properly exit with the same error code.

Signed-off-by: Martin Pitt 
---
 drivers/net/wireless/mac80211_hwsim.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 57361c3..a2224b7 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2183,7 +2183,12 @@ static int __init init_mac80211_hwsim(void)
if (IS_ERR(hwsim_class))
return PTR_ERR(hwsim_class);
 
-   driver_register(_hwsim_driver);
+   err = driver_register(_hwsim_driver);
+   if (err != 0) {
+   printk(KERN_DEBUG
+  "mac80211_hwsim: driver_register failed (%d)\n", err);
+   return err;
+   }
 
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
-- 
1.7.2.5

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

mac80211_hwsim: Check return value of driver_register

[Martin Pitt]

If driver_register() fails, properly exit with the same error code.

Signed-off-by: Martin Pitt martin.p...@ubuntu.com
---
 drivers/net/wireless/mac80211_hwsim.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 57361c3..a2224b7 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2183,7 +2183,12 @@ static int __init init_mac80211_hwsim(void)
if (IS_ERR(hwsim_class))
return PTR_ERR(hwsim_class);
 
-   driver_register(mac80211_hwsim_driver);
+   err = driver_register(mac80211_hwsim_driver);
+   if (err != 0) {
+   printk(KERN_DEBUG
+  mac80211_hwsim: driver_register failed (%d)\n, err);
+   return err;
+   }
 
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
-- 
1.7.2.5

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] mac80211_hwsim: Register and bind to driver

[Martin Pitt]

Properly register our mac80211_hwsim_driver, attach it to the platform bus.
Bind newly created hwsim devices to that driver, so that our wlan devices get
a proper "driver" sysfs attribute.

This makes mac80211_hwsim interfaces work with NetworkManager.

Signed-off-by: Martin Pitt 
---
 drivers/net/wireless/mac80211_hwsim.c |   18 +-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 0064d38..0a6fb4a 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1675,6 +1676,7 @@ static void mac80211_hwsim_free(void)
debugfs_remove(data->debugfs_ps);
debugfs_remove(data->debugfs);
ieee80211_unregister_hw(data->hw);
+   device_release_driver(data->dev);
device_unregister(data->dev);
ieee80211_free_hw(data->hw);
}
@@ -1683,7 +1685,9 @@ static void mac80211_hwsim_free(void)
 
 
 static struct device_driver mac80211_hwsim_driver = {
-   .name = "mac80211_hwsim"
+   .name = "mac80211_hwsim",
+   .bus = _bus_type,
+   .owner = THIS_MODULE,
 };
 
 static const struct net_device_ops hwsim_netdev_ops = {
@@ -2179,6 +2183,8 @@ static int __init init_mac80211_hwsim(void)
if (IS_ERR(hwsim_class))
return PTR_ERR(hwsim_class);
 
+   driver_register(_hwsim_driver);
+
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
 
@@ -2205,6 +2211,14 @@ static int __init init_mac80211_hwsim(void)
goto failed_drvdata;
}
data->dev->driver = _hwsim_driver;
+   err = device_bind_driver(data->dev);
+   if (err != 0) {
+   printk(KERN_DEBUG
+  "mac80211_hwsim: device_bind_driver "
+  "failed (%d)\n", err);
+   goto failed_hw;
+   }
+
skb_queue_head_init(>pending);
 
SET_IEEE80211_DEV(hw, data->dev);
@@ -2506,6 +2520,7 @@ failed_drvdata:
ieee80211_free_hw(hw);
 failed:
mac80211_hwsim_free();
+   driver_unregister(_hwsim_driver);
return err;
 }
 module_init(init_mac80211_hwsim);
@@ -2518,5 +2533,6 @@ static void __exit exit_mac80211_hwsim(void)
 
mac80211_hwsim_free();
unregister_netdev(hwsim_mon);
+   driver_unregister(_hwsim_driver);
 }
 module_exit(exit_mac80211_hwsim);
-- 
1.7.2.5

Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mac80211_hwsim: Register and bind to driver

[Martin Pitt]

Hello Johannes,

Johannes Berg [2013-04-08 11:12 +0200]:
> I think you forgot to update the error path to unregister the driver.

Oops, thanks for spotting. Sending [PATCH v2].

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] mac80211_hwsim: Register and bind to driver

[Martin Pitt]

Properly register our mac80211_hwsim_driver, attach it to the platform bus.
Bind newly created hwsim devices to that driver, so that our wlan devices get
a proper "driver" sysfs attribute.

This makes mac80211_hwsim interfaces work with NetworkManager.

Signed-off-by: Martin Pitt 
---
 drivers/net/wireless/mac80211_hwsim.c |   17 -
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 0064d38..50a8b52 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1675,6 +1676,7 @@ static void mac80211_hwsim_free(void)
debugfs_remove(data->debugfs_ps);
debugfs_remove(data->debugfs);
ieee80211_unregister_hw(data->hw);
+   device_release_driver(data->dev);
device_unregister(data->dev);
ieee80211_free_hw(data->hw);
}
@@ -1683,7 +1685,9 @@ static void mac80211_hwsim_free(void)
 
 
 static struct device_driver mac80211_hwsim_driver = {
-   .name = "mac80211_hwsim"
+   .name = "mac80211_hwsim",
+   .bus = _bus_type,
+   .owner = THIS_MODULE,
 };
 
 static const struct net_device_ops hwsim_netdev_ops = {
@@ -2179,6 +2183,8 @@ static int __init init_mac80211_hwsim(void)
if (IS_ERR(hwsim_class))
return PTR_ERR(hwsim_class);
 
+   driver_register(_hwsim_driver);
+
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
 
@@ -2205,6 +2211,14 @@ static int __init init_mac80211_hwsim(void)
goto failed_drvdata;
}
data->dev->driver = _hwsim_driver;
+   err = device_bind_driver(data->dev);
+   if (err != 0) {
+   printk(KERN_DEBUG
+  "mac80211_hwsim: device_bind_driver "
+  "failed (%d)\n", err);
+   goto failed_hw;
+   }
+
skb_queue_head_init(>pending);
 
SET_IEEE80211_DEV(hw, data->dev);
@@ -2518,5 +2532,6 @@ static void __exit exit_mac80211_hwsim(void)
 
mac80211_hwsim_free();
unregister_netdev(hwsim_mon);
+   driver_unregister(_hwsim_driver);
 }
 module_exit(exit_mac80211_hwsim);
-- 
1.7.2.5

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] mac80211_hwsim: Register and bind to driver

[Martin Pitt]

Properly register our mac80211_hwsim_driver, attach it to the platform bus.
Bind newly created hwsim devices to that driver, so that our wlan devices get
a proper driver sysfs attribute.

This makes mac80211_hwsim interfaces work with NetworkManager.

Signed-off-by: Martin Pitt martin.p...@ubuntu.com
---
 drivers/net/wireless/mac80211_hwsim.c |   17 -
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 0064d38..50a8b52 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -25,6 +25,7 @@
 #include linux/if_arp.h
 #include linux/rtnetlink.h
 #include linux/etherdevice.h
+#include linux/platform_device.h
 #include linux/debugfs.h
 #include linux/module.h
 #include linux/ktime.h
@@ -1675,6 +1676,7 @@ static void mac80211_hwsim_free(void)
debugfs_remove(data-debugfs_ps);
debugfs_remove(data-debugfs);
ieee80211_unregister_hw(data-hw);
+   device_release_driver(data-dev);
device_unregister(data-dev);
ieee80211_free_hw(data-hw);
}
@@ -1683,7 +1685,9 @@ static void mac80211_hwsim_free(void)
 
 
 static struct device_driver mac80211_hwsim_driver = {
-   .name = mac80211_hwsim
+   .name = mac80211_hwsim,
+   .bus = platform_bus_type,
+   .owner = THIS_MODULE,
 };
 
 static const struct net_device_ops hwsim_netdev_ops = {
@@ -2179,6 +2183,8 @@ static int __init init_mac80211_hwsim(void)
if (IS_ERR(hwsim_class))
return PTR_ERR(hwsim_class);
 
+   driver_register(mac80211_hwsim_driver);
+
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
 
@@ -2205,6 +2211,14 @@ static int __init init_mac80211_hwsim(void)
goto failed_drvdata;
}
data-dev-driver = mac80211_hwsim_driver;
+   err = device_bind_driver(data-dev);
+   if (err != 0) {
+   printk(KERN_DEBUG
+  mac80211_hwsim: device_bind_driver 
+  failed (%d)\n, err);
+   goto failed_hw;
+   }
+
skb_queue_head_init(data-pending);
 
SET_IEEE80211_DEV(hw, data-dev);
@@ -2518,5 +2532,6 @@ static void __exit exit_mac80211_hwsim(void)
 
mac80211_hwsim_free();
unregister_netdev(hwsim_mon);
+   driver_unregister(mac80211_hwsim_driver);
 }
 module_exit(exit_mac80211_hwsim);
-- 
1.7.2.5

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mac80211_hwsim: Register and bind to driver

[Martin Pitt]

Hello Johannes,

Johannes Berg [2013-04-08 11:12 +0200]:
 I think you forgot to update the error path to unregister the driver.

Oops, thanks for spotting. Sending [PATCH v2].

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] mac80211_hwsim: Register and bind to driver

[Martin Pitt]

Properly register our mac80211_hwsim_driver, attach it to the platform bus.
Bind newly created hwsim devices to that driver, so that our wlan devices get
a proper driver sysfs attribute.

This makes mac80211_hwsim interfaces work with NetworkManager.

Signed-off-by: Martin Pitt martin.p...@ubuntu.com
---
 drivers/net/wireless/mac80211_hwsim.c |   18 +-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 0064d38..0a6fb4a 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -25,6 +25,7 @@
 #include linux/if_arp.h
 #include linux/rtnetlink.h
 #include linux/etherdevice.h
+#include linux/platform_device.h
 #include linux/debugfs.h
 #include linux/module.h
 #include linux/ktime.h
@@ -1675,6 +1676,7 @@ static void mac80211_hwsim_free(void)
debugfs_remove(data-debugfs_ps);
debugfs_remove(data-debugfs);
ieee80211_unregister_hw(data-hw);
+   device_release_driver(data-dev);
device_unregister(data-dev);
ieee80211_free_hw(data-hw);
}
@@ -1683,7 +1685,9 @@ static void mac80211_hwsim_free(void)
 
 
 static struct device_driver mac80211_hwsim_driver = {
-   .name = mac80211_hwsim
+   .name = mac80211_hwsim,
+   .bus = platform_bus_type,
+   .owner = THIS_MODULE,
 };
 
 static const struct net_device_ops hwsim_netdev_ops = {
@@ -2179,6 +2183,8 @@ static int __init init_mac80211_hwsim(void)
if (IS_ERR(hwsim_class))
return PTR_ERR(hwsim_class);
 
+   driver_register(mac80211_hwsim_driver);
+
memset(addr, 0, ETH_ALEN);
addr[0] = 0x02;
 
@@ -2205,6 +2211,14 @@ static int __init init_mac80211_hwsim(void)
goto failed_drvdata;
}
data-dev-driver = mac80211_hwsim_driver;
+   err = device_bind_driver(data-dev);
+   if (err != 0) {
+   printk(KERN_DEBUG
+  mac80211_hwsim: device_bind_driver 
+  failed (%d)\n, err);
+   goto failed_hw;
+   }
+
skb_queue_head_init(data-pending);
 
SET_IEEE80211_DEV(hw, data-dev);
@@ -2506,6 +2520,7 @@ failed_drvdata:
ieee80211_free_hw(hw);
 failed:
mac80211_hwsim_free();
+   driver_unregister(mac80211_hwsim_driver);
return err;
 }
 module_init(init_mac80211_hwsim);
@@ -2518,5 +2533,6 @@ static void __exit exit_mac80211_hwsim(void)
 
mac80211_hwsim_free();
unregister_netdev(hwsim_mon);
+   driver_unregister(mac80211_hwsim_driver);
 }
 module_exit(exit_mac80211_hwsim);
-- 
1.7.2.5

Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/1] Option for scsi_debug to fake removable devices

[Martin Pitt]

Hello all,

I already re-sent this 1.5 months ago, but did not get any answer back
then; I guess it got lost in the noise by now. So, patiently retrying
again.

For the purposes of automatically testing udisks and gvfs automounting
I would like to add a parameter to scsi_debug to control the
"removable" attribute of the created block device. With that, we can
test system-internal and removable drives, as well as CD-ROMs (which
scsi_debug can already emulate). udisks requires different privileges
for mounting system-internal drives vs.  removable/hotpluggable
drives. This will also allow us to write system integration tests for
gvfs, which will exercise the whole stack including the actual polkit
configuration in a VM.

I wrote a simple kernel patch for this (against linux-next), and
tested this quite thoroughly.

I ran the style checker, and it reports two problems:

 8< --
WARNING: line over 80 characters
#109: FILE: drivers/scsi/scsi_debug.c:3255:
+   ret |= driver_create_file(_driverfs_driver, 
_attr_removable);

WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
#126: FILE: drivers/scsi/scsi_debug.c:3353:
+   printk(KERN_ERR "scsi_debug_init: removable must be 0 or 1\n");
 8< --

But as the existing code uses this style in the adjacent lines, I
favored consistency over fixing those. If the latter is desired, I'd
rather send a separate patch with just the style cleanup for the whole
file.

I got an ack from David Zeuthen (the primary udisks maintainer)
already, noted so in the patch.

Thank you in advance for considering,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

[PATCH 1/1] [SCSI] scsi_debug: Add "removable" parameter

[Martin Pitt]

Add "removable" module parameter to set the "removable" attribute of any
subsequently created debug block device. It is a writable driver option, so
that you can switch between removable and "fixed" media block devices in
between the add_host calls.

This is useful for being able to test the different behaviour/required
privileges in e. g. the udisks test suite.

Signed-off-by: Martin Pitt 
Acked-By: David Zeuthen 
---
 drivers/scsi/scsi_debug.c |   30 +++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..57fbd5a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -109,6 +109,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_OPT_BLKS 64
 #define DEF_PHYSBLK_EXP 0
 #define DEF_PTYPE   0
+#define DEF_REMOVABLE false
 #define DEF_SCSI_LEVEL   5/* INQUIRY, byte2 [5->SPC-3] */
 #define DEF_SECTOR_SIZE 512
 #define DEF_UNMAP_ALIGNMENT 0
@@ -193,11 +194,11 @@ static unsigned int scsi_debug_unmap_granularity = 
DEF_UNMAP_GRANULARITY;
 static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
 static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
 static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
+static bool scsi_debug_removable = DEF_REMOVABLE;
 
 static int scsi_debug_cmnd_count = 0;
 
 #define DEV_READONLY(TGT)  (0)
-#define DEV_REMOVEABLE(TGT)(0)
 
 static unsigned int sdebug_store_sectors;
 static sector_t sdebug_capacity;   /* in sectors */
@@ -919,7 +920,7 @@ static int resp_inquiry(struct scsi_cmnd * scp, int target,
return ret;
}
/* drops through here for a standard inquiry */
-   arr[1] = DEV_REMOVEABLE(target) ? 0x80 : 0; /* Removable disk */
+   arr[1] = scsi_debug_removable ? 0x80 : 0;   /* Removable disk */
arr[2] = scsi_debug_scsi_level;
arr[3] = 2;/* response_data_format==2 */
arr[4] = SDEBUG_LONG_INQ_SZ - 5;
@@ -1211,7 +1212,7 @@ static int resp_format_pg(unsigned char * p, int 
pcontrol, int target)
p[11] = sdebug_sectors_per & 0xff;
p[12] = (scsi_debug_sector_size >> 8) & 0xff;
p[13] = scsi_debug_sector_size & 0xff;
-   if (DEV_REMOVEABLE(target))
+   if (scsi_debug_removable)
p[20] |= 0x20; /* should agree with INQUIRY */
if (1 == pcontrol)
memset(p + 2, 0, sizeof(format_pg) - 2);
@@ -2754,6 +2755,7 @@ module_param_named(opt_blks, scsi_debug_opt_blks, int, 
S_IRUGO);
 module_param_named(opts, scsi_debug_opts, int, S_IRUGO | S_IWUSR);
 module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
 module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
+module_param_named(removable, scsi_debug_removable, bool, S_IRUGO | S_IWUSR);
 module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
 module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
 module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
@@ -2796,6 +2798,7 @@ MODULE_PARM_DESC(opt_blks, "optimal transfer length in 
block (def=64)");
 MODULE_PARM_DESC(opts, "1->noise, 2->medium_err, 4->timeout, 
8->recovered_err... (def=0)");
 MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
 MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
+MODULE_PARM_DESC(removable, "claim to have removable media (def=0)");
 MODULE_PARM_DESC(scsi_level, "SCSI level to simulate(def=5[SPC-3])");
 MODULE_PARM_DESC(sector_size, "logical block size in bytes (def=512)");
 MODULE_PARM_DESC(unmap_alignment, "lowest aligned thin provisioning lba 
(def=0)");
@@ -3205,6 +3208,25 @@ static ssize_t sdebug_map_show(struct device_driver 
*ddp, char *buf)
 }
 DRIVER_ATTR(map, S_IRUGO, sdebug_map_show, NULL);
 
+static ssize_t sdebug_removable_show(struct device_driver *ddp,
+char *buf)
+{
+   return scnprintf(buf, PAGE_SIZE, "%d\n", scsi_debug_removable ? 1 : 0);
+}
+static ssize_t sdebug_removable_store(struct device_driver *ddp,
+ const char *buf, size_t count)
+{
+   int n;
+
+   if ((count > 0) && (1 == sscanf(buf, "%d", )) && (n >= 0)) {
+   scsi_debug_removable = (n > 0);
+   return count;
+   }
+   return -EINVAL;
+}
+DRIVER_ATTR(removable, S_IRUGO | S_IWUSR, sdebug_removable_show,
+   sdebug_removable_store);
+
 
 /* Note: The following function creates attribute files in the
/sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
@@ -3230,6 +3252,7 @@ static int do_create_driverfs_files(void)
ret |= driver_create_file(_driverfs_driver, 
_attr_num_tgts);
 

[PATCH 1/1] [SCSI] scsi_debug: Add removable parameter

[Martin Pitt]

Add removable module parameter to set the removable attribute of any
subsequently created debug block device. It is a writable driver option, so
that you can switch between removable and fixed media block devices in
between the add_host calls.

This is useful for being able to test the different behaviour/required
privileges in e. g. the udisks test suite.

Signed-off-by: Martin Pitt martin.p...@ubuntu.com
Acked-By: David Zeuthen zeut...@gmail.com
---
 drivers/scsi/scsi_debug.c |   30 +++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..57fbd5a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -109,6 +109,7 @@ static const char * scsi_debug_version_date = 20100324;
 #define DEF_OPT_BLKS 64
 #define DEF_PHYSBLK_EXP 0
 #define DEF_PTYPE   0
+#define DEF_REMOVABLE false
 #define DEF_SCSI_LEVEL   5/* INQUIRY, byte2 [5-SPC-3] */
 #define DEF_SECTOR_SIZE 512
 #define DEF_UNMAP_ALIGNMENT 0
@@ -193,11 +194,11 @@ static unsigned int scsi_debug_unmap_granularity = 
DEF_UNMAP_GRANULARITY;
 static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
 static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
 static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
+static bool scsi_debug_removable = DEF_REMOVABLE;
 
 static int scsi_debug_cmnd_count = 0;
 
 #define DEV_READONLY(TGT)  (0)
-#define DEV_REMOVEABLE(TGT)(0)
 
 static unsigned int sdebug_store_sectors;
 static sector_t sdebug_capacity;   /* in sectors */
@@ -919,7 +920,7 @@ static int resp_inquiry(struct scsi_cmnd * scp, int target,
return ret;
}
/* drops through here for a standard inquiry */
-   arr[1] = DEV_REMOVEABLE(target) ? 0x80 : 0; /* Removable disk */
+   arr[1] = scsi_debug_removable ? 0x80 : 0;   /* Removable disk */
arr[2] = scsi_debug_scsi_level;
arr[3] = 2;/* response_data_format==2 */
arr[4] = SDEBUG_LONG_INQ_SZ - 5;
@@ -1211,7 +1212,7 @@ static int resp_format_pg(unsigned char * p, int 
pcontrol, int target)
p[11] = sdebug_sectors_per  0xff;
p[12] = (scsi_debug_sector_size  8)  0xff;
p[13] = scsi_debug_sector_size  0xff;
-   if (DEV_REMOVEABLE(target))
+   if (scsi_debug_removable)
p[20] |= 0x20; /* should agree with INQUIRY */
if (1 == pcontrol)
memset(p + 2, 0, sizeof(format_pg) - 2);
@@ -2754,6 +2755,7 @@ module_param_named(opt_blks, scsi_debug_opt_blks, int, 
S_IRUGO);
 module_param_named(opts, scsi_debug_opts, int, S_IRUGO | S_IWUSR);
 module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
 module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
+module_param_named(removable, scsi_debug_removable, bool, S_IRUGO | S_IWUSR);
 module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
 module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
 module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
@@ -2796,6 +2798,7 @@ MODULE_PARM_DESC(opt_blks, optimal transfer length in 
block (def=64));
 MODULE_PARM_DESC(opts, 1-noise, 2-medium_err, 4-timeout, 
8-recovered_err... (def=0));
 MODULE_PARM_DESC(physblk_exp, physical block exponent (def=0));
 MODULE_PARM_DESC(ptype, SCSI peripheral type(def=0[disk]));
+MODULE_PARM_DESC(removable, claim to have removable media (def=0));
 MODULE_PARM_DESC(scsi_level, SCSI level to simulate(def=5[SPC-3]));
 MODULE_PARM_DESC(sector_size, logical block size in bytes (def=512));
 MODULE_PARM_DESC(unmap_alignment, lowest aligned thin provisioning lba 
(def=0));
@@ -3205,6 +3208,25 @@ static ssize_t sdebug_map_show(struct device_driver 
*ddp, char *buf)
 }
 DRIVER_ATTR(map, S_IRUGO, sdebug_map_show, NULL);
 
+static ssize_t sdebug_removable_show(struct device_driver *ddp,
+char *buf)
+{
+   return scnprintf(buf, PAGE_SIZE, %d\n, scsi_debug_removable ? 1 : 0);
+}
+static ssize_t sdebug_removable_store(struct device_driver *ddp,
+ const char *buf, size_t count)
+{
+   int n;
+
+   if ((count  0)  (1 == sscanf(buf, %d, n))  (n = 0)) {
+   scsi_debug_removable = (n  0);
+   return count;
+   }
+   return -EINVAL;
+}
+DRIVER_ATTR(removable, S_IRUGO | S_IWUSR, sdebug_removable_show,
+   sdebug_removable_store);
+
 
 /* Note: The following function creates attribute files in the
/sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
@@ -3230,6 +3252,7 @@ static int do_create_driverfs_files(void)
ret |= driver_create_file(sdebug_driverfs_driver, 
driver_attr_num_tgts);
ret |= driver_create_file(sdebug_driverfs_driver, driver_attr_ptype);
ret |= driver_create_file(sdebug_driverfs_driver, driver_attr_opts);
+   ret |= driver_create_file

[PATCH 0/1] Option for scsi_debug to fake removable devices

[Martin Pitt]

Hello all,

I already re-sent this 1.5 months ago, but did not get any answer back
then; I guess it got lost in the noise by now. So, patiently retrying
again.

For the purposes of automatically testing udisks and gvfs automounting
I would like to add a parameter to scsi_debug to control the
removable attribute of the created block device. With that, we can
test system-internal and removable drives, as well as CD-ROMs (which
scsi_debug can already emulate). udisks requires different privileges
for mounting system-internal drives vs.  removable/hotpluggable
drives. This will also allow us to write system integration tests for
gvfs, which will exercise the whole stack including the actual polkit
configuration in a VM.

I wrote a simple kernel patch for this (against linux-next), and
tested this quite thoroughly.

I ran the style checker, and it reports two problems:

 8 --
WARNING: line over 80 characters
#109: FILE: drivers/scsi/scsi_debug.c:3255:
+   ret |= driver_create_file(sdebug_driverfs_driver, 
driver_attr_removable);

WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
#126: FILE: drivers/scsi/scsi_debug.c:3353:
+   printk(KERN_ERR scsi_debug_init: removable must be 0 or 1\n);
 8 --

But as the existing code uses this style in the adjacent lines, I
favored consistency over fixing those. If the latter is desired, I'd
rather send a separate patch with just the style cleanup for the whole
file.

I got an ack from David Zeuthen (the primary udisks maintainer)
already, noted so in the patch.

Thank you in advance for considering,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

[PATCH 1/1] [SCSI] scsi_debug: Add "removable" parameter

[Martin Pitt]

Add "removable" module parameter to set the "removable" attribute of any
subsequently created debug block device. It is a writable driver option, so
that you can switch between removable and "fixed" media block devices in
between the add_host calls.

This is useful for being able to test the different behaviour/required
privileges in e. g. the udisks test suite.

Signed-off-by: Martin Pitt 
Acked-By: David Zeuthen 
---
 drivers/scsi/scsi_debug.c |   30 +++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..57fbd5a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -109,6 +109,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_OPT_BLKS 64
 #define DEF_PHYSBLK_EXP 0
 #define DEF_PTYPE   0
+#define DEF_REMOVABLE false
 #define DEF_SCSI_LEVEL   5/* INQUIRY, byte2 [5->SPC-3] */
 #define DEF_SECTOR_SIZE 512
 #define DEF_UNMAP_ALIGNMENT 0
@@ -193,11 +194,11 @@ static unsigned int scsi_debug_unmap_granularity = 
DEF_UNMAP_GRANULARITY;
 static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
 static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
 static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
+static bool scsi_debug_removable = DEF_REMOVABLE;
 
 static int scsi_debug_cmnd_count = 0;
 
 #define DEV_READONLY(TGT)  (0)
-#define DEV_REMOVEABLE(TGT)(0)
 
 static unsigned int sdebug_store_sectors;
 static sector_t sdebug_capacity;   /* in sectors */
@@ -919,7 +920,7 @@ static int resp_inquiry(struct scsi_cmnd * scp, int target,
return ret;
}
/* drops through here for a standard inquiry */
-   arr[1] = DEV_REMOVEABLE(target) ? 0x80 : 0; /* Removable disk */
+   arr[1] = scsi_debug_removable ? 0x80 : 0;   /* Removable disk */
arr[2] = scsi_debug_scsi_level;
arr[3] = 2;/* response_data_format==2 */
arr[4] = SDEBUG_LONG_INQ_SZ - 5;
@@ -1211,7 +1212,7 @@ static int resp_format_pg(unsigned char * p, int 
pcontrol, int target)
p[11] = sdebug_sectors_per & 0xff;
p[12] = (scsi_debug_sector_size >> 8) & 0xff;
p[13] = scsi_debug_sector_size & 0xff;
-   if (DEV_REMOVEABLE(target))
+   if (scsi_debug_removable)
p[20] |= 0x20; /* should agree with INQUIRY */
if (1 == pcontrol)
memset(p + 2, 0, sizeof(format_pg) - 2);
@@ -2754,6 +2755,7 @@ module_param_named(opt_blks, scsi_debug_opt_blks, int, 
S_IRUGO);
 module_param_named(opts, scsi_debug_opts, int, S_IRUGO | S_IWUSR);
 module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
 module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
+module_param_named(removable, scsi_debug_removable, bool, S_IRUGO | S_IWUSR);
 module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
 module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
 module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
@@ -2796,6 +2798,7 @@ MODULE_PARM_DESC(opt_blks, "optimal transfer length in 
block (def=64)");
 MODULE_PARM_DESC(opts, "1->noise, 2->medium_err, 4->timeout, 
8->recovered_err... (def=0)");
 MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
 MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
+MODULE_PARM_DESC(removable, "claim to have removable media (def=0)");
 MODULE_PARM_DESC(scsi_level, "SCSI level to simulate(def=5[SPC-3])");
 MODULE_PARM_DESC(sector_size, "logical block size in bytes (def=512)");
 MODULE_PARM_DESC(unmap_alignment, "lowest aligned thin provisioning lba 
(def=0)");
@@ -3205,6 +3208,25 @@ static ssize_t sdebug_map_show(struct device_driver 
*ddp, char *buf)
 }
 DRIVER_ATTR(map, S_IRUGO, sdebug_map_show, NULL);
 
+static ssize_t sdebug_removable_show(struct device_driver *ddp,
+char *buf)
+{
+   return scnprintf(buf, PAGE_SIZE, "%d\n", scsi_debug_removable ? 1 : 0);
+}
+static ssize_t sdebug_removable_store(struct device_driver *ddp,
+ const char *buf, size_t count)
+{
+   int n;
+
+   if ((count > 0) && (1 == sscanf(buf, "%d", )) && (n >= 0)) {
+   scsi_debug_removable = (n > 0);
+   return count;
+   }
+   return -EINVAL;
+}
+DRIVER_ATTR(removable, S_IRUGO | S_IWUSR, sdebug_removable_show,
+   sdebug_removable_store);
+
 
 /* Note: The following function creates attribute files in the
/sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
@@ -3230,6 +3252,7 @@ static int do_create_driverfs_files(void)
ret |= driver_create_file(_driverfs_driver, 
_attr_num_tgts);
 

[PATCH 0/1] Option for scsi_debug to fake removable devices

[Martin Pitt]

Hello all,

sorry for resending this twice to the lists. Initially I thought
mailing James directly was not appropriate, but I was now told that
get_maintainer.pl is actually speaking the truth and I am supposed to
do just that.


For the purposes of automatically testing udisks and gvfs automounting
I would like to add a parameter to scsi_debug to control the
"removable" attribute of the created block device. With that, we can
test system-internal and removable drives, as well as CD-ROMs (which
scsi_debug can already emulate). udisks requires different privileges
for mounting system-internal drives vs.  removable/hotpluggable
drives. This will also allow us to write system integration tests for
gvfs, which will exercise the whole stack including the actual polkit
configuration in a VM.

I wrote a simple kernel patch for this (against linux-next), and
tested this quite thoroughly.

I ran the style checker, and it reports two problems:

 8< --
WARNING: line over 80 characters
#109: FILE: drivers/scsi/scsi_debug.c:3255:
+   ret |= driver_create_file(_driverfs_driver, 
_attr_removable);

WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
#126: FILE: drivers/scsi/scsi_debug.c:3353:
+   printk(KERN_ERR "scsi_debug_init: removable must be 0 or 1\n");
 8< --

But as the existing code uses this style in the adjacent lines, I
favored consistency over fixing those. If the latter is desired, I'd
rather send a separate patch with just the style cleanup for the whole
file.

I got an ack from David Zeuthen (the primary udisks maintainer)
already, noted so in the patch.

Thank you in advance for considering,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

[PATCH 0/1] Option for scsi_debug to fake removable devices

[Martin Pitt]

Hello all,

sorry for resending this twice to the lists. Initially I thought
mailing James directly was not appropriate, but I was now told that
get_maintainer.pl is actually speaking the truth and I am supposed to
do just that.


For the purposes of automatically testing udisks and gvfs automounting
I would like to add a parameter to scsi_debug to control the
removable attribute of the created block device. With that, we can
test system-internal and removable drives, as well as CD-ROMs (which
scsi_debug can already emulate). udisks requires different privileges
for mounting system-internal drives vs.  removable/hotpluggable
drives. This will also allow us to write system integration tests for
gvfs, which will exercise the whole stack including the actual polkit
configuration in a VM.

I wrote a simple kernel patch for this (against linux-next), and
tested this quite thoroughly.

I ran the style checker, and it reports two problems:

 8 --
WARNING: line over 80 characters
#109: FILE: drivers/scsi/scsi_debug.c:3255:
+   ret |= driver_create_file(sdebug_driverfs_driver, 
driver_attr_removable);

WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
#126: FILE: drivers/scsi/scsi_debug.c:3353:
+   printk(KERN_ERR scsi_debug_init: removable must be 0 or 1\n);
 8 --

But as the existing code uses this style in the adjacent lines, I
favored consistency over fixing those. If the latter is desired, I'd
rather send a separate patch with just the style cleanup for the whole
file.

I got an ack from David Zeuthen (the primary udisks maintainer)
already, noted so in the patch.

Thank you in advance for considering,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

[PATCH 1/1] [SCSI] scsi_debug: Add removable parameter

[Martin Pitt]

Add removable module parameter to set the removable attribute of any
subsequently created debug block device. It is a writable driver option, so
that you can switch between removable and fixed media block devices in
between the add_host calls.

This is useful for being able to test the different behaviour/required
privileges in e. g. the udisks test suite.

Signed-off-by: Martin Pitt martin.p...@ubuntu.com
Acked-By: David Zeuthen zeut...@gmail.com
---
 drivers/scsi/scsi_debug.c |   30 +++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..57fbd5a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -109,6 +109,7 @@ static const char * scsi_debug_version_date = 20100324;
 #define DEF_OPT_BLKS 64
 #define DEF_PHYSBLK_EXP 0
 #define DEF_PTYPE   0
+#define DEF_REMOVABLE false
 #define DEF_SCSI_LEVEL   5/* INQUIRY, byte2 [5-SPC-3] */
 #define DEF_SECTOR_SIZE 512
 #define DEF_UNMAP_ALIGNMENT 0
@@ -193,11 +194,11 @@ static unsigned int scsi_debug_unmap_granularity = 
DEF_UNMAP_GRANULARITY;
 static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
 static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
 static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
+static bool scsi_debug_removable = DEF_REMOVABLE;
 
 static int scsi_debug_cmnd_count = 0;
 
 #define DEV_READONLY(TGT)  (0)
-#define DEV_REMOVEABLE(TGT)(0)
 
 static unsigned int sdebug_store_sectors;
 static sector_t sdebug_capacity;   /* in sectors */
@@ -919,7 +920,7 @@ static int resp_inquiry(struct scsi_cmnd * scp, int target,
return ret;
}
/* drops through here for a standard inquiry */
-   arr[1] = DEV_REMOVEABLE(target) ? 0x80 : 0; /* Removable disk */
+   arr[1] = scsi_debug_removable ? 0x80 : 0;   /* Removable disk */
arr[2] = scsi_debug_scsi_level;
arr[3] = 2;/* response_data_format==2 */
arr[4] = SDEBUG_LONG_INQ_SZ - 5;
@@ -1211,7 +1212,7 @@ static int resp_format_pg(unsigned char * p, int 
pcontrol, int target)
p[11] = sdebug_sectors_per  0xff;
p[12] = (scsi_debug_sector_size  8)  0xff;
p[13] = scsi_debug_sector_size  0xff;
-   if (DEV_REMOVEABLE(target))
+   if (scsi_debug_removable)
p[20] |= 0x20; /* should agree with INQUIRY */
if (1 == pcontrol)
memset(p + 2, 0, sizeof(format_pg) - 2);
@@ -2754,6 +2755,7 @@ module_param_named(opt_blks, scsi_debug_opt_blks, int, 
S_IRUGO);
 module_param_named(opts, scsi_debug_opts, int, S_IRUGO | S_IWUSR);
 module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
 module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
+module_param_named(removable, scsi_debug_removable, bool, S_IRUGO | S_IWUSR);
 module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
 module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
 module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
@@ -2796,6 +2798,7 @@ MODULE_PARM_DESC(opt_blks, optimal transfer length in 
block (def=64));
 MODULE_PARM_DESC(opts, 1-noise, 2-medium_err, 4-timeout, 
8-recovered_err... (def=0));
 MODULE_PARM_DESC(physblk_exp, physical block exponent (def=0));
 MODULE_PARM_DESC(ptype, SCSI peripheral type(def=0[disk]));
+MODULE_PARM_DESC(removable, claim to have removable media (def=0));
 MODULE_PARM_DESC(scsi_level, SCSI level to simulate(def=5[SPC-3]));
 MODULE_PARM_DESC(sector_size, logical block size in bytes (def=512));
 MODULE_PARM_DESC(unmap_alignment, lowest aligned thin provisioning lba 
(def=0));
@@ -3205,6 +3208,25 @@ static ssize_t sdebug_map_show(struct device_driver 
*ddp, char *buf)
 }
 DRIVER_ATTR(map, S_IRUGO, sdebug_map_show, NULL);
 
+static ssize_t sdebug_removable_show(struct device_driver *ddp,
+char *buf)
+{
+   return scnprintf(buf, PAGE_SIZE, %d\n, scsi_debug_removable ? 1 : 0);
+}
+static ssize_t sdebug_removable_store(struct device_driver *ddp,
+ const char *buf, size_t count)
+{
+   int n;
+
+   if ((count  0)  (1 == sscanf(buf, %d, n))  (n = 0)) {
+   scsi_debug_removable = (n  0);
+   return count;
+   }
+   return -EINVAL;
+}
+DRIVER_ATTR(removable, S_IRUGO | S_IWUSR, sdebug_removable_show,
+   sdebug_removable_store);
+
 
 /* Note: The following function creates attribute files in the
/sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
@@ -3230,6 +3252,7 @@ static int do_create_driverfs_files(void)
ret |= driver_create_file(sdebug_driverfs_driver, 
driver_attr_num_tgts);
ret |= driver_create_file(sdebug_driverfs_driver, driver_attr_ptype);
ret |= driver_create_file(sdebug_driverfs_driver, driver_attr_opts);
+   ret |= driver_create_file

Providing an ELF flag for disabling LD_PRELOAD/ptrace()

[Martin Pitt]

Hi kernel developers,

one thing that has bothered me for a long time already is the
complete lack of a security boundary between processes of the same
user. Things like LD_PRELOAD and ptrace() (IOW, gdb) are enabled by
default for all users, and especially for developers this is a good
thing.

However, a lot of programs that we have deal with passwords and other
secrets which deserve some protection, like passwords you type into
ssh, screensavers, seahorse, etc.

This problem has become more pressing with the advent of PolicyKit,
where even fewer processes have the in-built privilege separation
between root and users.

This concerns a scenario where you might have a rogue trojan in your
session (e. g. a malicious firefox plugin or an autostart shell
script). Admittedly, if an attacker gets that far he has almost won
the machine and can easily get more privileges with some social
engineering and spoofing, but I feel it is worth the small effort to
at least not allow reading passwords from other processes' memory
without making any noise at all. This would also greatly reduce the
potential of a local trojan spreading itself over existing ssh
connections to other hosts (provided that the ssh executable is
protected like that, which it should be anyway because it deals with
passwords).

What I want is the behaviour of suid/sgid executables (which do
something like an atomic prctl(PR_SET_DUMPABLE, 0) to disable vectors
like ptrace(), LD_PRELOAD, etc. However, making binaries setugid just
for that is less than ideal, since it requires a lot of code patching
(to reset the group) and packaging changes (to maintain the
sgid setting), as well as confusing security scanners, etc.

So I wonder whether we can define a flag in the ELF header which
triggers the same behaviour? Can we define an e_flags bit for that?

Thanks in advance for any comment or idea,

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Providing an ELF flag for disabling LD_PRELOAD/ptrace()

[Martin Pitt]

Hi kernel developers,

one thing that has bothered me for a long time already is the
complete lack of a security boundary between processes of the same
user. Things like LD_PRELOAD and ptrace() (IOW, gdb) are enabled by
default for all users, and especially for developers this is a good
thing.

However, a lot of programs that we have deal with passwords and other
secrets which deserve some protection, like passwords you type into
ssh, screensavers, seahorse, etc.

This problem has become more pressing with the advent of PolicyKit,
where even fewer processes have the in-built privilege separation
between root and users.

This concerns a scenario where you might have a rogue trojan in your
session (e. g. a malicious firefox plugin or an autostart shell
script). Admittedly, if an attacker gets that far he has almost won
the machine and can easily get more privileges with some social
engineering and spoofing, but I feel it is worth the small effort to
at least not allow reading passwords from other processes' memory
without making any noise at all. This would also greatly reduce the
potential of a local trojan spreading itself over existing ssh
connections to other hosts (provided that the ssh executable is
protected like that, which it should be anyway because it deals with
passwords).

What I want is the behaviour of suid/sgid executables (which do
something like an atomic prctl(PR_SET_DUMPABLE, 0) to disable vectors
like ptrace(), LD_PRELOAD, etc. However, making binaries setugid just
for that is less than ideal, since it requires a lot of code patching
(to reset the group) and packaging changes (to maintain the
sgid setting), as well as confusing security scanners, etc.

So I wonder whether we can define a flag in the ELF header which
triggers the same behaviour? Can we define an e_flags bit for that?

Thanks in advance for any comment or idea,

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Martin Pitt]

Hi Eugene,

Eugene Teo [2007-07-29 21:03 +0800]:
> >> Also, it is probably good to think how we can "drop privileges" while 
> >> piping
> >> the core dump output to an external program. A malicious user can 
> >> potentially
> >> use it as a possible backdoor since anything that is executed by 
> >> "|program" will
> >> be executed with root privileges.
> >>
> > It was my understanding that apport already did this.
> 
> I haven't looked at apport yet, but are you talking about the userspace 
> portion of
> apport or the kernel changes in the Ubuntu kernel?

Similarly to Neil's patches, the Ubuntu kernel calls the userspace
helper as root, too. Apport drops privileges to the target process as
soon as possible (there are a few things it needs to do before, like
opening an fd to the crash file in /var/crash/ if that is only
writeable by root).

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

Hi Neil,

Neil Horman [2007-07-28 13:21 -0400]:
> Jeremy asked that I make a patch next week to address split_argv's requirement
> that the argc parameter be non-NULL.  I'll be fixing that next week, and what 
> I
> can do is further enhance it such that it ignores spaces in quoted strings,
> which should address the case that concerns you.  I.E I can make split_argv
> behave such that:
> echo "|\"foo bar\" --pid %p" > /proc/sys/kernel/core_pattern
> results in the following argv:
> {{"foo bar"}, {"--pid"}, {"1234"}}
> 
> Which I think handles what you are looking for.

Oh, handling escaping and quoting is going to make it fairly
complicated, but sure, if you need that for other things, too, that
would solve the remaining case. I just wonder if, instead of
implementing escaping, it wouldn't be easier to first split on spaces
and then escape macros?

> Thank you for clearing me up on this.  So it would seem we're ok with what we
> have now, correct?  

Absolutely, yes.

> We just have a potential corner case to address, which I can
> reasonably handle with a modification to split_argv, that I have a
> todo on next week.

Right, it's really just for perfectionism. Spaces in executable names
are EBW anyway, and readlink()ing /proc//exe is much more robust
anyway in terms of a small and orthogonal interface.

If the upstream kernel guys don't worry about it and consider it a
blocker for merging, I don't either. :-)

Thanks a lot,

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

Hi Neil,

Neil Horman [2007-07-28 13:21 -0400]:
 Jeremy asked that I make a patch next week to address split_argv's requirement
 that the argc parameter be non-NULL.  I'll be fixing that next week, and what 
 I
 can do is further enhance it such that it ignores spaces in quoted strings,
 which should address the case that concerns you.  I.E I can make split_argv
 behave such that:
 echo |\foo bar\ --pid %p  /proc/sys/kernel/core_pattern
 results in the following argv:
 {{foo bar}, {--pid}, {1234}}
 
 Which I think handles what you are looking for.

Oh, handling escaping and quoting is going to make it fairly
complicated, but sure, if you need that for other things, too, that
would solve the remaining case. I just wonder if, instead of
implementing escaping, it wouldn't be easier to first split on spaces
and then escape macros?

 Thank you for clearing me up on this.  So it would seem we're ok with what we
 have now, correct?  

Absolutely, yes.

 We just have a potential corner case to address, which I can
 reasonably handle with a modification to split_argv, that I have a
 todo on next week.

Right, it's really just for perfectionism. Spaces in executable names
are EBW anyway, and readlink()ing /proc/pid/exe is much more robust
anyway in terms of a small and orthogonal interface.

If the upstream kernel guys don't worry about it and consider it a
blocker for merging, I don't either. :-)

Thanks a lot,

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Martin Pitt]

Hi Eugene,

Eugene Teo [2007-07-29 21:03 +0800]:
  Also, it is probably good to think how we can drop privileges while 
  piping
  the core dump output to an external program. A malicious user can 
  potentially
  use it as a possible backdoor since anything that is executed by 
  |program will
  be executed with root privileges.
 
  It was my understanding that apport already did this.
 
 I haven't looked at apport yet, but are you talking about the userspace 
 portion of
 apport or the kernel changes in the Ubuntu kernel?

Similarly to Neil's patches, the Ubuntu kernel calls the userspace
helper as root, too. Apport drops privileges to the target process as
soon as possible (there are a few things it needs to do before, like
opening an fd to the crash file in /var/crash/ if that is only
writeable by root).

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

Hi Neil,

Neil Horman [2007-07-28  9:46 -0400]:
> > I just want to mention a potential problem with this: If you first
> > expand the macros (from pattern to corename) and then split
> > corename into an argv, then this breaks macro expansions
> > containing spaces.  This mostly affects the executable name, of
> > course.
> > 
> I never intended for this core_pattern argument passing to be able
> to expand macros, other than the macros specified by the
> core_pattern code.  If you want it to do that, we can address that
> with a later patch.

No, I specifically mean the standard ones provided by
format_corename(), such as %p (pid), %s (signal), or %e (executable
name). I don't see a reason to provide additional functionality.

> > In fact we considered this macro approach when doing the original
> > patches in the Ubuntu kernel, but we eventually used environment
> > variables because they are much easier and more robust to
> > implement than doing a robust macro expansion (i. e. first split
> > core_pattern into an argv and then call the macro expansion for
> > each element).
> > 
> I disagree. While it might be nice to be able to specify environment
> variables as command line arguments, it would be much easier to just
> let the core_pattern executable inherit the crashing processes
> environment.  we don't do that currently, but we easily could.  That
> way any information that the crashing process wants the dying
> process to know can be inherited and vetted easily by apport (or
> whatever the core_pattern points to).  I'll do a patch later for
> that if you don't like it.

I don't think that this will be necessary. After all, the crash
handler can read all the environment from /proc// (and that's
indeed what apport does to figure out relevant parts from the
environment like the locale).

It seems we misunderstood each other, I don't expect or want any new
functionality in core_pattern. AN example might make it more clear:

The original problem that we are trying to solve is the current
behaviour of core_pattern expansion with pipes:

  |/bin/crash --pid %p

would try to execute the program '/bin/crash --pid 1234' instead of
calling /bin/crash with ['--pid', '1234'] as argv, right? Your patch
achieves the latter by splitting the formatted core dump string into
an array (at spaces).

I pointed out that this leads to problems when macro values contain
spaces. This currently affects hostname (%h) (although this really
should not happen in practice) and executable name (%e) (rare, but at
least valid).  I. e. for an executable name "foo bar" your patch would
expand

  |/bin/crash %e

to ['/bin/crash', 'foo', 'bar'] instead of ['/bin/crash', 'foo bar'].

Of course this is a corner case, and personally I don't really care.
I strive to keep the assumptions about the interface at a minimum, so
right now Apport's only required input is the core dump itself (over
stdin); signal and pid can be read from the environment, and if not
present, they are read from the core dump.

I did not defend Ubuntu's usage of environment variables, on the
contrary. Using the standard macros is more explicit and elegant, and
I welcome that change. I just pointed out the reason why we chose the
environment variable approach initially.

I just wanted to mention this little problem for the sake of
correctness.

Thank you, and have a nice weekend!

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

Hi Neil,

thanks a lot for your work on this!

Neil Horman [2007-07-27 16:08 -0400]:
> Hey
>   Patch 2/3 of my core_pattern enhancements.  This patch is a rewrite of
> my previous post for this enhancement.  It uses jeremy's split_argv/free_argv
> library functions to translate core_pattern into an argv array to be passed to
> the user mode helper process.  
> [...]
>   if (ispipe) {
>   core_limit = RLIM_INFINITY;
> + helper_argv = argv_split(GFP_KERNEL, corename+1, _argc);

I just want to mention a potential problem with this: If you first
expand the macros (from pattern to corename) and then split corename
into an argv, then this breaks macro expansions containing spaces.
This mostly affects the executable name, of course.

In fact we considered this macro approach when doing the original
patches in the Ubuntu kernel, but we eventually used environment
variables because they are much easier and more robust to implement
than doing a robust macro expansion (i. e. first split core_pattern
into an argv and then call the macro expansion for each element).

I would love to use macros instead since it looks a bit cleaner, and
personally I do not care about the "executable name" macro for Apport
[1] (I grab it from /proc/pid/exe), but I wanted to mention this
possible caveat before it goes upstream.

Thank you,

Martin

[1] https://wiki.ubuntu.com/Apport

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

Hi Neil,

thanks a lot for your work on this!

Neil Horman [2007-07-27 16:08 -0400]:
 Hey
   Patch 2/3 of my core_pattern enhancements.  This patch is a rewrite of
 my previous post for this enhancement.  It uses jeremy's split_argv/free_argv
 library functions to translate core_pattern into an argv array to be passed to
 the user mode helper process.  
 [...]
   if (ispipe) {
   core_limit = RLIM_INFINITY;
 + helper_argv = argv_split(GFP_KERNEL, corename+1, helper_argc);

I just want to mention a potential problem with this: If you first
expand the macros (from pattern to corename) and then split corename
into an argv, then this breaks macro expansions containing spaces.
This mostly affects the executable name, of course.

In fact we considered this macro approach when doing the original
patches in the Ubuntu kernel, but we eventually used environment
variables because they are much easier and more robust to implement
than doing a robust macro expansion (i. e. first split core_pattern
into an argv and then call the macro expansion for each element).

I would love to use macros instead since it looks a bit cleaner, and
personally I do not care about the executable name macro for Apport
[1] (I grab it from /proc/pid/exe), but I wanted to mention this
possible caveat before it goes upstream.

Thank you,

Martin

[1] https://wiki.ubuntu.com/Apport

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

Hi Neil,

Neil Horman [2007-07-28  9:46 -0400]:
  I just want to mention a potential problem with this: If you first
  expand the macros (from pattern to corename) and then split
  corename into an argv, then this breaks macro expansions
  containing spaces.  This mostly affects the executable name, of
  course.
  
 I never intended for this core_pattern argument passing to be able
 to expand macros, other than the macros specified by the
 core_pattern code.  If you want it to do that, we can address that
 with a later patch.

No, I specifically mean the standard ones provided by
format_corename(), such as %p (pid), %s (signal), or %e (executable
name). I don't see a reason to provide additional functionality.

  In fact we considered this macro approach when doing the original
  patches in the Ubuntu kernel, but we eventually used environment
  variables because they are much easier and more robust to
  implement than doing a robust macro expansion (i. e. first split
  core_pattern into an argv and then call the macro expansion for
  each element).
  
 I disagree. While it might be nice to be able to specify environment
 variables as command line arguments, it would be much easier to just
 let the core_pattern executable inherit the crashing processes
 environment.  we don't do that currently, but we easily could.  That
 way any information that the crashing process wants the dying
 process to know can be inherited and vetted easily by apport (or
 whatever the core_pattern points to).  I'll do a patch later for
 that if you don't like it.

I don't think that this will be necessary. After all, the crash
handler can read all the environment from /proc/pid/ (and that's
indeed what apport does to figure out relevant parts from the
environment like the locale).

It seems we misunderstood each other, I don't expect or want any new
functionality in core_pattern. AN example might make it more clear:

The original problem that we are trying to solve is the current
behaviour of core_pattern expansion with pipes:

  |/bin/crash --pid %p

would try to execute the program '/bin/crash --pid 1234' instead of
calling /bin/crash with ['--pid', '1234'] as argv, right? Your patch
achieves the latter by splitting the formatted core dump string into
an array (at spaces).

I pointed out that this leads to problems when macro values contain
spaces. This currently affects hostname (%h) (although this really
should not happen in practice) and executable name (%e) (rare, but at
least valid).  I. e. for an executable name foo bar your patch would
expand

  |/bin/crash %e

to ['/bin/crash', 'foo', 'bar'] instead of ['/bin/crash', 'foo bar'].

Of course this is a corner case, and personally I don't really care.
I strive to keep the assumptions about the interface at a minimum, so
right now Apport's only required input is the core dump itself (over
stdin); signal and pid can be read from the environment, and if not
present, they are read from the core dump.

I did not defend Ubuntu's usage of environment variables, on the
contrary. Using the standard macros is more explicit and elegant, and
I welcome that change. I just pointed out the reason why we chose the
environment variable approach initially.

I just wanted to mention this little problem for the sake of
correctness.

Thank you, and have a nice weekend!

Martin

-- 
Martin Pitthttp://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org


signature.asc
Description: Digital signature