netdev - Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150611153430.GA2302@nanopsycho.orion>
Date:	Thu, 11 Jun 2015 17:34:31 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Scott Feldman <sfeldma@...il.com>
Cc:	David Ahern <dsahern@...il.com>, Netdev <netdev@...r.kernel.org>,
	Brenden Blanco <bblanco@...mgrid.com>
Subject: Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't
 support set attr op

Thu, Jun 11, 2015 at 05:03:21PM CEST, sfeldma@...il.com wrote:
>On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Thu, Jun 11, 2015 at 12:00:47AM CEST, sfeldma@...il.com wrote:
>>>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman <sfeldma@...il.com> wrote:
>>>> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsahern@...il.com> wrote:
>>>>> On 6/10/15 2:56 PM, sfeldma@...il.com wrote:
>>>>>>
>>>>>> From: Scott Feldman <sfeldma@...il.com>
>>>>>>
>>>>>> Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged
>>>>>> port does not support switchdec_port_attr_set op.  Don't BUG() if
>>>>>> -EOPNOTSUPP is returned.
>>>>>>
>>>>>> Signed-off-by: Scott Feldman <sfeldma@...il.com>
>>>>>> Reported-by: Brenden Blanco <bblanco@...mgrid.com>
>>>>>> ---
>>>>>>   net/switchdev/switchdev.c |    2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>>>>>> index e008057..99bced4 100644
>>>>>> --- a/net/switchdev/switchdev.c
>>>>>> +++ b/net/switchdev/switchdev.c
>>>>>> @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct
>>>>>> work_struct *work)
>>>>>>
>>>>>>         rtnl_lock();
>>>>>>         err = switchdev_port_attr_set(asw->dev, &asw->attr);
>>>>>> -       BUG_ON(err);
>>>>>> +       BUG_ON(err && err != -EOPNOTSUPP);
>>>>>>         rtnl_unlock();
>>>>>>
>>>>>>         dev_put(asw->dev);
>>>>>>
>>>>>
>>>>> Should that be WARN_ON instead of BUG_ON?
>>>>
>>>> I think I had it as WARN when we were working on the initial patches,
>>>> but we changed it to BUG_ON because we should only get an error here
>>>> if the driver screwed something up between PREPARE phase and COMMIT
>>>> phase, so it should be considered a driver bug which needs fixing.
>>>
>>>Actually, ignore what I said above.  I was confusing this BUG_ON with
>>>the one in switchdev_port_attr_set().  Perhaps this BUG_ON() you're
>>>commenting on should be WARN().  A driver could return an err in
>>>PREPARE phase and that shouldn't be a BUG_ON situation; seems WARN
>>>would be better.   It the case where the driver returns an err in
>>>COMMIT phase but didn't return an err in PREPARE phase we want to
>>>BUG_ON().  Maybe that case doesn't justify BUG_ON either, based on the
>>>link you posted.
>>>
>>>Jiri, IIRC, you suggested the BUG_ON().  Does it still sound right
>>>based on the point David is raising?
>>
>> Hmm, looking at code of switchdev_port_attr_set. In case that fails in
>> prepare state (which can easily happen for example due to -ENOMEM) this
>> BUG_ON is hit as well. That is not right. I think we should change it
>> just to warning. Also I think that prink (or a flavour) is more suitable
>> here than WARN.
>
>Thanks, I'll change it to netdev_err.
>
>> Btw, why switchdev_port_obj_add has WARN and not BUG ?
>
>Not sure.  We should be consistent. WARN seems better for both
>obj_add/attr_set than BUG, given the link David Ahern posted.  Do you
>agree?

Okay. Sounds reasonable.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html