[PATCH] r8152: correct error returns

[PATCH] r8152: correct error returns

[Oliver Neukum]

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(-)

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:
-- 
1.8.4.5

Re: [PATCH] r8152: correct error returns

[David Miller]

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.

Re: [PATCH] r8152: correct error returns

[Oliver Neukum]

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

Re: [PATCH] r8152: correct error returns

[David Miller]

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.

Re: [PATCH] r8152: correct error returns

[Oliver Neukum]

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

Re: [PATCH] r8152: correct error returns

[David Miller]

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.