Re: [patch net-next v2 01/28] net: dev: Check CHANGEUPPER notifier return value
On 12/02/2015 09:07 PM, Jiri Pirko wrote: > From: Ido Schimmel > > switchdev drivers reflect the newly requested topology to hardware when > CHANGEUPPER is received, after software links were already formed. > However, the operation can fail and user will not be notified, as the > return value of the notifier is not checked. > > Add this check and rollback software links if necessary. > > Signed-off-by: Ido Schimmel > Signed-off-by: Jiri Pirko > --- > v1->v2: > -new patch > --- Reviewed-by: Nikolay Aleksandrov -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 01/28] net: dev: Check CHANGEUPPER notifier return value
On 15-12-02 12:07 PM, Jiri Pirko wrote: > From: Ido Schimmel > > switchdev drivers reflect the newly requested topology to hardware when > CHANGEUPPER is received, after software links were already formed. > However, the operation can fail and user will not be notified, as the > return value of the notifier is not checked. > > Add this check and rollback software links if necessary. > > Signed-off-by: Ido Schimmel > Signed-off-by: Jiri Pirko > --- > v1->v2: > -new patch > --- > net/core/dev.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 5df6cbc..df33f82 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -5490,8 +5490,11 @@ static int __netdev_upper_dev_link(struct net_device > *dev, > goto rollback_lower_mesh; > } > > - call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev, > - &changeupper_info.info); > + ret = call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev, > + &changeupper_info.info); > + if (ret) > + goto rollback_lower_mesh; > + hmm small nit (take it or leave it) but I think it would be more correct if this was if (ret == NOTIFY_BAD) goto rollback_lower_mesh; It seems that NOTIFY_DONE, NOTIFY_OK and NOTIFY_STOP_MASK would be valid return codes that don't indicate an error. However seeing I couldn't find any cases of NOTIFY_OK/NOTIFY_STOP_MASK from the CHANGEUPPER event it doesn't matter in practice. Thanks, John > return 0; > > rollback_lower_mesh: > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 01/28] net: dev: Check CHANGEUPPER notifier return value
On Thu, Dec 3, 2015 at 1:20 AM, John Fastabend wrote: > On 15-12-02 12:07 PM, Jiri Pirko wrote: >> From: Ido Schimmel >> >> switchdev drivers reflect the newly requested topology to hardware when >> CHANGEUPPER is received, after software links were already formed. >> However, the operation can fail and user will not be notified, as the >> return value of the notifier is not checked. >> >> Add this check and rollback software links if necessary. >> net/core/dev.c | 7 +-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 5df6cbc..df33f82 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -5490,8 +5490,11 @@ static int __netdev_upper_dev_link(struct net_device >> *dev, >> goto rollback_lower_mesh; >> } >> >> - call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev, >> - &changeupper_info.info); >> + ret = call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev, >> + &changeupper_info.info); >> + if (ret) >> + goto rollback_lower_mesh; >> + > > hmm small nit (take it or leave it) but I think it would be more > correct if this was > > if (ret == NOTIFY_BAD) > goto rollback_lower_mesh; > > It seems that NOTIFY_DONE, NOTIFY_OK and NOTIFY_STOP_MASK would be > valid return codes that don't indicate an error. However seeing I > couldn't find any cases of NOTIFY_OK/NOTIFY_STOP_MASK from the > CHANGEUPPER event it doesn't matter in practice. When saying that you couldn't find any such cases in the current code, can't they be added in the future? and hence following your suggestion would eliminate future wrong behaviour, right? >> return 0; >> >> rollback_lower_mesh: -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 01/28] net: dev: Check CHANGEUPPER notifier return value
Thu, Dec 03, 2015 at 01:20:50AM IST, john.fastab...@gmail.com wrote:
>On 15-12-02 12:07 PM, Jiri Pirko wrote:
>> From: Ido Schimmel
>>
>> switchdev drivers reflect the newly requested topology to hardware when
>> CHANGEUPPER is received, after software links were already formed.
>> However, the operation can fail and user will not be notified, as the
>> return value of the notifier is not checked.
>>
>> Add this check and rollback software links if necessary.
>>
>> Signed-off-by: Ido Schimmel
>> Signed-off-by: Jiri Pirko
>> ---
>> v1->v2:
>> -new patch
>> ---
>> net/core/dev.c | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 5df6cbc..df33f82 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5490,8 +5490,11 @@ static int __netdev_upper_dev_link(struct net_device
>> *dev,
>> goto rollback_lower_mesh;
>> }
>>
>> -call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
>> - &changeupper_info.info);
>> +ret = call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
>> +&changeupper_info.info);
>> +if (ret)
>> +goto rollback_lower_mesh;
>> +
>
>hmm small nit (take it or leave it) but I think it would be more
>correct if this was
>
> if (ret == NOTIFY_BAD)
> goto rollback_lower_mesh;
>
>It seems that NOTIFY_DONE, NOTIFY_OK and NOTIFY_STOP_MASK would be
>valid return codes that don't indicate an error. However seeing I
>couldn't find any cases of NOTIFY_OK/NOTIFY_STOP_MASK from the
>CHANGEUPPER event it doesn't matter in practice.
>
Hi,
It's actually not a small nit. I forgot to call 'notifier_to_errno' and
then check its retrun value instead that of the notifier. This would
return 0 for NOTIFY_{DONE,OK,STOP_MASK} and -1 for NOTIFY_BAD.
Thank you.
>Thanks,
>John
>
>> return 0;
>>
>> rollback_lower_mesh:
>>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
