[PATCH] bonding: avoid re-entry of bond_release

[PATCH] bonding: avoid re-entry of bond_release

[Wengang Wang]

If bond_release is run against an interface which is already detached from
it's master, then there is an error message shown like
	"<master name> cannot release <slave name>".

The call path is:
	bond_do_ioctl()
		bond_release()
			__bond_release_one()

Though it does not really harm, the message the message is misleading.
This patch tries to avoid the message.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 drivers/net/bonding/bond_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 184c434..4a71bbd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
 		break;
 	case BOND_RELEASE_OLD:
 	case SIOCBONDRELEASE:
-		res = bond_release(bond_dev, slave_dev);
+		if (slave_dev->flags & IFF_SLAVE)
+			res = bond_release(bond_dev, slave_dev);
+		else
+			res = 0;
 		break;
 	case BOND_SETHWADDR_OLD:
 	case SIOCBONDSETHWADDR:
-- 
1.8.3.1

Re: [PATCH] bonding: avoid re-entry of bond_release

[Andy Gospodarek]

On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
> If bond_release is run against an interface which is already detached from
> it's master, then there is an error message shown like
> 	"<master name> cannot release <slave name>".
> 
> The call path is:
> 	bond_do_ioctl()
> 		bond_release()
> 			__bond_release_one()
> 
> Though it does not really harm, the message the message is misleading.
> This patch tries to avoid the message.
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  drivers/net/bonding/bond_main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 184c434..4a71bbd 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>  		break;
>  	case BOND_RELEASE_OLD:
>  	case SIOCBONDRELEASE:
> -		res = bond_release(bond_dev, slave_dev);
> +		if (slave_dev->flags & IFF_SLAVE)
> +			res = bond_release(bond_dev, slave_dev);
> +		else
> +			res = 0;

Functionally this patch is fine, but I would prefer that you simply
change the check in __bond_release_one to not be so noisy.  There is a
check[1] in bond_enslave to see if a slave is already in a bond and that
just prints a message of netdev_dbg (rather than netdev_err) and it
seems that would be appropriate for this type of message.

[1] from bond_enslave():

        /* already enslaved */
        if (slave_dev->flags & IFF_SLAVE) {
                netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
                return -EBUSY;
        }


>  		break;
>  	case BOND_SETHWADDR_OLD:
>  	case SIOCBONDSETHWADDR:
> -- 
> 1.8.3.1
> 
> --
> 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

Re: [PATCH] bonding: avoid re-entry of bond_release

[Ding Tianhong]

On 2014/12/19 23:11, Andy Gospodarek wrote:
> On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
>> If bond_release is run against an interface which is already detached from
>> it's master, then there is an error message shown like
>> 	"<master name> cannot release <slave name>".
>>
>> The call path is:
>> 	bond_do_ioctl()
>> 		bond_release()
>> 			__bond_release_one()
>>
>> Though it does not really harm, the message the message is misleading.
>> This patch tries to avoid the message.
>>
>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> ---
>>  drivers/net/bonding/bond_main.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 184c434..4a71bbd 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>>  		break;
>>  	case BOND_RELEASE_OLD:
>>  	case SIOCBONDRELEASE:
>> -		res = bond_release(bond_dev, slave_dev);
>> +		if (slave_dev->flags & IFF_SLAVE)
>> +			res = bond_release(bond_dev, slave_dev);
>> +		else
>> +			res = 0;
> 
> Functionally this patch is fine, but I would prefer that you simply
> change the check in __bond_release_one to not be so noisy.  There is a
> check[1] in bond_enslave to see if a slave is already in a bond and that
> just prints a message of netdev_dbg (rather than netdev_err) and it
> seems that would be appropriate for this type of message.
> 
> [1] from bond_enslave():
> 
>         /* already enslaved */
>         if (slave_dev->flags & IFF_SLAVE) {
>                 netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>                 return -EBUSY;
>         }
> 
> 
>>  		break;
>>  	case BOND_SETHWADDR_OLD:
>>  	case SIOCBONDSETHWADDR:
>> -- 

agree ,use netdev_dbg looks more better and enough.

Ding

>> 1.8.3.1
>>
>> --
>> 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
> --
> 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
> 
> 

Re: [PATCH] bonding: avoid re-entry of bond_release

[Wengang]

Hi Andy and Ding,

Thanks for your reviews!
In the ioctl path, removing a interface that is not currently actually a 
slave
can happen from user space(by mistake), we should avoid the noisy message.

While, __bond_release_one() has another call path which is from 
bond_uninit().
In the later case, it should be treated as an error if the interface is 
not with
IFF_SLAVE flag. To notice that error occurred, the message is printed. I 
think
the message is needed for this path.

How do you think?

thanks,
wengang

于 2014年12月21日 10:01, Ding Tianhong 写道:
> On 2014/12/19 23:11, Andy Gospodarek wrote:
>> On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
>>> If bond_release is run against an interface which is already detached from
>>> it's master, then there is an error message shown like
>>> 	"<master name> cannot release <slave name>".
>>>
>>> The call path is:
>>> 	bond_do_ioctl()
>>> 		bond_release()
>>> 			__bond_release_one()
>>>
>>> Though it does not really harm, the message the message is misleading.
>>> This patch tries to avoid the message.
>>>
>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>> ---
>>>   drivers/net/bonding/bond_main.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 184c434..4a71bbd 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>>>   		break;
>>>   	case BOND_RELEASE_OLD:
>>>   	case SIOCBONDRELEASE:
>>> -		res = bond_release(bond_dev, slave_dev);
>>> +		if (slave_dev->flags & IFF_SLAVE)
>>> +			res = bond_release(bond_dev, slave_dev);
>>> +		else
>>> +			res = 0;
>> Functionally this patch is fine, but I would prefer that you simply
>> change the check in __bond_release_one to not be so noisy.  There is a
>> check[1] in bond_enslave to see if a slave is already in a bond and that
>> just prints a message of netdev_dbg (rather than netdev_err) and it
>> seems that would be appropriate for this type of message.
>>
>> [1] from bond_enslave():
>>
>>          /* already enslaved */
>>          if (slave_dev->flags & IFF_SLAVE) {
>>                  netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>>                  return -EBUSY;
>>          }
>>
>>
>>>   		break;
>>>   	case BOND_SETHWADDR_OLD:
>>>   	case SIOCBONDSETHWADDR:
>>> -- 
> agree ,use netdev_dbg looks more better and enough.
>
> Ding
>
>

Re: [PATCH] bonding: avoid re-entry of bond_release

[Ding Tianhong]

On 2014/12/22 9:09, Wengang wrote:
> Hi Andy and Ding,
> 
> Thanks for your reviews!
> In the ioctl path, removing a interface that is not currently actually a slave
> can happen from user space(by mistake), we should avoid the noisy message.
> 
> While, __bond_release_one() has another call path which is from bond_uninit().
> In the later case, it should be treated as an error if the interface is not with
> IFF_SLAVE flag. To notice that error occurred, the message is printed. I think
> the message is needed for this path.
> 
> How do you think?
> 

Just like the bond_enslave(), it is only a warning.

Ding

> thanks,
> wengang
> 
> 于 2014年12月21日 10:01, Ding Tianhong 写道:
>> On 2014/12/19 23:11, Andy Gospodarek wrote:
>>> On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
>>>> If bond_release is run against an interface which is already detached from
>>>> it's master, then there is an error message shown like
>>>>     "<master name> cannot release <slave name>".
>>>>
>>>> The call path is:
>>>>     bond_do_ioctl()
>>>>         bond_release()
>>>>             __bond_release_one()
>>>>
>>>> Though it does not really harm, the message the message is misleading.
>>>> This patch tries to avoid the message.
>>>>
>>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>>> ---
>>>>   drivers/net/bonding/bond_main.c | 5 ++++-
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>> index 184c434..4a71bbd 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>>>>           break;
>>>>       case BOND_RELEASE_OLD:
>>>>       case SIOCBONDRELEASE:
>>>> -        res = bond_release(bond_dev, slave_dev);
>>>> +        if (slave_dev->flags & IFF_SLAVE)
>>>> +            res = bond_release(bond_dev, slave_dev);
>>>> +        else
>>>> +            res = 0;
>>> Functionally this patch is fine, but I would prefer that you simply
>>> change the check in __bond_release_one to not be so noisy.  There is a
>>> check[1] in bond_enslave to see if a slave is already in a bond and that
>>> just prints a message of netdev_dbg (rather than netdev_err) and it
>>> seems that would be appropriate for this type of message.
>>>
>>> [1] from bond_enslave():
>>>
>>>          /* already enslaved */
>>>          if (slave_dev->flags & IFF_SLAVE) {
>>>                  netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>>>                  return -EBUSY;
>>>          }
>>>
>>>
>>>>           break;
>>>>       case BOND_SETHWADDR_OLD:
>>>>       case SIOCBONDSETHWADDR:
>>>> -- 
>> agree ,use netdev_dbg looks more better and enough.
>>
>> Ding
>>
>>
> 
> 
> .
> 

Re: [PATCH] bonding: avoid re-entry of bond_release

[Wengang]

OK. Will change as suggested and re-post.

thanks,
wengang

于 2014年12月22日 10:05, Ding Tianhong 写道:
> On 2014/12/22 9:09, Wengang wrote:
>> Hi Andy and Ding,
>>
>> Thanks for your reviews!
>> In the ioctl path, removing a interface that is not currently actually a slave
>> can happen from user space(by mistake), we should avoid the noisy message.
>>
>> While, __bond_release_one() has another call path which is from bond_uninit().
>> In the later case, it should be treated as an error if the interface is not with
>> IFF_SLAVE flag. To notice that error occurred, the message is printed. I think
>> the message is needed for this path.
>>
>> How do you think?
>>
> Just like the bond_enslave(), it is only a warning.
>
> Ding
>
>> thanks,
>> wengang
>>
>> 于 2014年12月21日 10:01, Ding Tianhong 写道:
>>> On 2014/12/19 23:11, Andy Gospodarek wrote:
>>>> On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
>>>>> If bond_release is run against an interface which is already detached from
>>>>> it's master, then there is an error message shown like
>>>>>      "<master name> cannot release <slave name>".
>>>>>
>>>>> The call path is:
>>>>>      bond_do_ioctl()
>>>>>          bond_release()
>>>>>              __bond_release_one()
>>>>>
>>>>> Though it does not really harm, the message the message is misleading.
>>>>> This patch tries to avoid the message.
>>>>>
>>>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>>>> ---
>>>>>    drivers/net/bonding/bond_main.c | 5 ++++-
>>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>>> index 184c434..4a71bbd 100644
>>>>> --- a/drivers/net/bonding/bond_main.c
>>>>> +++ b/drivers/net/bonding/bond_main.c
>>>>> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>>>>>            break;
>>>>>        case BOND_RELEASE_OLD:
>>>>>        case SIOCBONDRELEASE:
>>>>> -        res = bond_release(bond_dev, slave_dev);
>>>>> +        if (slave_dev->flags & IFF_SLAVE)
>>>>> +            res = bond_release(bond_dev, slave_dev);
>>>>> +        else
>>>>> +            res = 0;
>>>> Functionally this patch is fine, but I would prefer that you simply
>>>> change the check in __bond_release_one to not be so noisy.  There is a
>>>> check[1] in bond_enslave to see if a slave is already in a bond and that
>>>> just prints a message of netdev_dbg (rather than netdev_err) and it
>>>> seems that would be appropriate for this type of message.
>>>>
>>>> [1] from bond_enslave():
>>>>
>>>>           /* already enslaved */
>>>>           if (slave_dev->flags & IFF_SLAVE) {
>>>>                   netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>>>>                   return -EBUSY;
>>>>           }
>>>>
>>>>
>>>>>            break;
>>>>>        case BOND_SETHWADDR_OLD:
>>>>>        case SIOCBONDSETHWADDR:
>>>>> -- 
>>> agree ,use netdev_dbg looks more better and enough.
>>>
>>> Ding
>>>
>>>
>>

Re: [PATCH] bonding: avoid re-entry of bond_release

[Andy Gospodarek]

On Mon, Dec 22, 2014 at 04:30:40PM +0800, Wengang wrote:
> OK. Will change as suggested and re-post.

Sounds great.  Thanks for your work on this.

> 
> thanks,
> wengang
> 
> 于 2014年12月22日 10:05, Ding Tianhong 写道:
> >On 2014/12/22 9:09, Wengang wrote:
> >>Hi Andy and Ding,
> >>
> >>Thanks for your reviews!
> >>In the ioctl path, removing a interface that is not currently actually a slave
> >>can happen from user space(by mistake), we should avoid the noisy message.
> >>
> >>While, __bond_release_one() has another call path which is from bond_uninit().
> >>In the later case, it should be treated as an error if the interface is not with
> >>IFF_SLAVE flag. To notice that error occurred, the message is printed. I think
> >>the message is needed for this path.
> >>
> >>How do you think?
> >>
> >Just like the bond_enslave(), it is only a warning.
> >
> >Ding
> >
> >>thanks,
> >>wengang
> >>
> >>于 2014年12月21日 10:01, Ding Tianhong 写道:
> >>>On 2014/12/19 23:11, Andy Gospodarek wrote:
> >>>>On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
> >>>>>If bond_release is run against an interface which is already detached from
> >>>>>it's master, then there is an error message shown like
> >>>>>     "<master name> cannot release <slave name>".
> >>>>>
> >>>>>The call path is:
> >>>>>     bond_do_ioctl()
> >>>>>         bond_release()
> >>>>>             __bond_release_one()
> >>>>>
> >>>>>Though it does not really harm, the message the message is misleading.
> >>>>>This patch tries to avoid the message.
> >>>>>
> >>>>>Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >>>>>---
> >>>>>   drivers/net/bonding/bond_main.c | 5 ++++-
> >>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >>>>>index 184c434..4a71bbd 100644
> >>>>>--- a/drivers/net/bonding/bond_main.c
> >>>>>+++ b/drivers/net/bonding/bond_main.c
> >>>>>@@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
> >>>>>           break;
> >>>>>       case BOND_RELEASE_OLD:
> >>>>>       case SIOCBONDRELEASE:
> >>>>>-        res = bond_release(bond_dev, slave_dev);
> >>>>>+        if (slave_dev->flags & IFF_SLAVE)
> >>>>>+            res = bond_release(bond_dev, slave_dev);
> >>>>>+        else
> >>>>>+            res = 0;
> >>>>Functionally this patch is fine, but I would prefer that you simply
> >>>>change the check in __bond_release_one to not be so noisy.  There is a
> >>>>check[1] in bond_enslave to see if a slave is already in a bond and that
> >>>>just prints a message of netdev_dbg (rather than netdev_err) and it
> >>>>seems that would be appropriate for this type of message.
> >>>>
> >>>>[1] from bond_enslave():
> >>>>
> >>>>          /* already enslaved */
> >>>>          if (slave_dev->flags & IFF_SLAVE) {
> >>>>                  netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
> >>>>                  return -EBUSY;
> >>>>          }
> >>>>
> >>>>
> >>>>>           break;
> >>>>>       case BOND_SETHWADDR_OLD:
> >>>>>       case SIOCBONDSETHWADDR:
> >>>>>-- 
> >>>agree ,use netdev_dbg looks more better and enough.
> >>>
> >>>Ding
> >>>
> >>>
> >>
>