| Message ID | 1406901370-6847-1-git-send-email-oneukum@suse.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Oliver Neukum <oneukum@suse.de> Date: Fri, 1 Aug 2014 15:56:10 +0200 > If an autoresume fails the internal error codes of the PM > subsystem mustn't be leaked to user space. Replace them by EIO > > Signed-off-by: Oliver Neukum <oneukum@suse.de> Really? Then even the core usbnet driver gets this wrong, as well as many other drivers I scanned over for this problem. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2014-08-02 at 16:34 -0700, David Miller wrote: > From: Oliver Neukum <oneukum@suse.de> > Date: Fri, 1 Aug 2014 15:56:10 +0200 > > > If an autoresume fails the internal error codes of the PM > > subsystem mustn't be leaked to user space. Replace them by EIO > > > > Signed-off-by: Oliver Neukum <oneukum@suse.de> > > Really? Yes. You can get results like returning -EINVAL from open(). You can call down into drivers/core/power/runtime.c::static int rpm_resume(struct device *dev, int rpmflags) As you can see it uses errors in its own ways: static int rpm_resume(struct device *dev, int rpmflags) __releases(&dev->power.lock) __acquires(&dev->power.lock) { int (*callback)(struct device *); struct device *parent = NULL; int retval = 0; trace_rpm_resume(dev, rpmflags); repeat: if (dev->power.runtime_error) retval = -EINVAL; else if (dev->power.disable_depth == 1 && dev->power.is_suspended && dev->power.runtime_status == RPM_ACTIVE) retval = 1; else if (dev->power.disable_depth > 0) retval = -EACCES; if (retval) goto out; The call chain is usb_autopm_get_interface()->pm_runtime_get_sync()->__pm_runtime_resume() ->rpm_resume() > Then even the core usbnet driver gets this wrong, as well as many > other drivers I scanned over for this problem. OK, I just looked at this single driver. I'll try to come up with a generic solution. The mapping is a bit simplistic. I should let -ENOMEM pass for example. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Neukum <oneukum@suse.de> Date: Sun, 03 Aug 2014 09:06:58 +0200 > OK, I just looked at this single driver. I'll try to come up with > a generic solution. The mapping is a bit simplistic. I should let > -ENOMEM pass for example. And the problem also isn't limited USB networking drivers, just about every driver I looked at passed these autopm errors right back to userspace. Largely people just call the autopm interfaces and propagate any errors they get. So you probably want to put the translator/filter there. But of course, that doesn't handle the non-autopm code paths into the PM layer that can end up in this situation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-08-04 at 15:18 -0700, David Miller wrote: > From: Oliver Neukum <oneukum@suse.de> > Date: Sun, 03 Aug 2014 09:06:58 +0200 > > > OK, I just looked at this single driver. I'll try to come up with > > a generic solution. The mapping is a bit simplistic. I should let > > -ENOMEM pass for example. > > And the problem also isn't limited USB networking drivers, just > about every driver I looked at passed these autopm errors right > back to userspace. I drilled a gas bubble. > Largely people just call the autopm interfaces and propagate any > errors they get. So you probably want to put the translator/filter > there. That would swallow up all error returns and hurt the users who actually use the error codes. How about a native and a sanitized version? > But of course, that doesn't handle the non-autopm code paths into the > PM layer that can end up in this situation. Hm. Are they numerous? Which did you find? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Neukum <oneukum@suse.de> Date: Tue, 05 Aug 2014 09:43:04 +0200 > On Mon, 2014-08-04 at 15:18 -0700, David Miller wrote: >> But of course, that doesn't handle the non-autopm code paths into the >> PM layer that can end up in this situation. > Hm. Are they numerous? Which did you find? I didn't find any specifically, I just know that there are other callers of that inner-most helper function so those paths need to be checked too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 87f7104..0e5b9f9 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -2810,6 +2810,7 @@ static int rtl8152_open(struct net_device *netdev) res = usb_autopm_get_interface(tp->intf); if (res < 0) { free_all_mem(tp); + res = -EIO; goto out; } @@ -3116,8 +3117,10 @@ static int rtl8152_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) int ret; ret = usb_autopm_get_interface(tp->intf); - if (ret < 0) + if (ret < 0) { + ret = -EIO; goto out_set_wol; + } __rtl_set_wol(tp, wol->wolopts); tp->saved_wolopts = wol->wolopts & WAKE_ANY; @@ -3169,8 +3172,10 @@ static int rtl8152_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) int ret; ret = usb_autopm_get_interface(tp->intf); - if (ret < 0) + if (ret < 0) { + ret = -EIO; goto out; + } ret = rtl8152_set_speed(tp, cmd->autoneg, cmd->speed, cmd->duplex); @@ -3267,8 +3272,10 @@ static int rtl8152_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd) return -ENODEV; res = usb_autopm_get_interface(tp->intf); - if (res < 0) + if (res < 0) { + res = -EIO; goto out; + } switch (cmd) { case SIOCGMIIPHY:
If an autoresume fails the internal error codes of the PM subsystem mustn't be leaked to user space. Replace them by EIO Signed-off-by: Oliver Neukum <oneukum@suse.de> --- drivers/net/usb/r8152.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)