[PATCH net-next] bridge: fix setlink/dellink notifications

[PATCH net-next] bridge: fix setlink/dellink notifications

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

problems with bridge getlink/setlink notifications today:
        - bridge setlink generates two notifications to userspace
                - one from the bridge driver
                - one from rtnetlink.c (rtnl_bridge_notify)
        - dellink generates one notification from rtnetlink.c. Which
	means bridge setlink and dellink notifications are not
	consistent

        - Looking at the code it appears,
	If both BRIDGE_FLAGS_MASTER and BRIDGE_FLAGS_SELF were set,
        the size calculation in rtnl_bridge_notify can be wrong.
        Example: if you set both BRIDGE_FLAGS_MASTER and BRIDGE_FLAGS_SELF
        in a setlink request to rocker dev, rtnl_bridge_notify will
	allocate skb for one set of bridge attributes, but,
	both the bridge driver and rocker dev will try to add
	attributes resulting in twice the number of attributes
	being added to the skb.  (rocker dev calls ndo_dflt_bridge_getlink)

There are multiple options:
1) Generate one notification including all attributes from master and self:
   But, I don't think it will work, because both master and self may use
   the same attributes/policy. Cannot pack the same set of attributes in a
   single notification from both master and slave (duplicate attributes).

2) Generate one notification from master and the other notification from
   self (This seems to be ideal):
     For master: the master driver will send notification (bridge in this
	example)
     For self: the self driver will send notification (rocker in the above
	example. It can use helpers from rtnetlink.c to do so. Like the
	ndo_dflt_bridge_getlink api).

This patch implements 2) (leaving the 'rtnl_bridge_notify' around to be used
with 'self').

CC'ing others who might be affected by this change for review.

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/bridge/br_netlink.c |    2 ++
 net/core/rtnetlink.c    |   32 ++++++++++++++++----------------
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 9f5eb55..169783a 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -432,6 +432,8 @@ int br_dellink(struct net_device *dev, struct nlmsghdr *nlh)
 
 	err = br_afspec((struct net_bridge *)netdev_priv(dev), p,
 			afspec, RTM_DELLINK);
+	if (err == 0)
+		br_ifinfo_notify(RTM_DELLINK, p);
 
 	return err;
 }
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d06107d..4ac79ff 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2876,13 +2876,6 @@ static int rtnl_bridge_notify(struct net_device *dev, u16 flags)
 		goto errout;
 	}
 
-	if ((!flags || (flags & BRIDGE_FLAGS_MASTER)) &&
-	    br_dev && br_dev->netdev_ops->ndo_bridge_getlink) {
-		err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
-		if (err < 0)
-			goto errout;
-	}
-
 	if ((flags & BRIDGE_FLAGS_SELF) &&
 	    dev->netdev_ops->ndo_bridge_getlink) {
 		err = dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
@@ -2958,16 +2951,19 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh)
 			err = -EOPNOTSUPP;
 		else
 			err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
-
-		if (!err)
+		if (!err) {
 			flags &= ~BRIDGE_FLAGS_SELF;
+
+			/* Generate event to notify upper layer of bridge
+			 * change
+			 */
+			if (!err)
+				err = rtnl_bridge_notify(dev, oflags);
+		}
 	}
 
 	if (have_flags)
 		memcpy(nla_data(attr), &flags, sizeof(flags));
-	/* Generate event to notify upper layer of bridge change */
-	if (!err)
-		err = rtnl_bridge_notify(dev, oflags);
 out:
 	return err;
 }
@@ -3032,15 +3028,19 @@ static int rtnl_bridge_dellink(struct sk_buff *skb, struct nlmsghdr *nlh)
 		else
 			err = dev->netdev_ops->ndo_bridge_dellink(dev, nlh);
 
-		if (!err)
+		if (!err) {
 			flags &= ~BRIDGE_FLAGS_SELF;
+
+			/* Generate event to notify upper layer of bridge
+			 * change
+			 */
+			err = rtnl_bridge_notify(dev, oflags);
+		}
+
 	}
 
 	if (have_flags)
 		memcpy(nla_data(attr), &flags, sizeof(flags));
-	/* Generate event to notify upper layer of bridge change */
-	if (!err)
-		err = rtnl_bridge_notify(dev, oflags);
 out:
 	return err;
 }
-- 
1.7.10.4

Re: [PATCH net-next] bridge: fix setlink/dellink notifications

[Thomas Graf]

On 01/13/15 at 10:48pm, roopa@cumulusnetworks.com wrote:
> 2) Generate one notification from master and the other notification from
>    self (This seems to be ideal):
>      For master: the master driver will send notification (bridge in this
> 	example)
>      For self: the self driver will send notification (rocker in the above
> 	example. It can use helpers from rtnetlink.c to do so. Like the
> 	ndo_dflt_bridge_getlink api).
> 
> This patch implements 2) (leaving the 'rtnl_bridge_notify' around to be used
> with 'self').
> 
> CC'ing others who might be affected by this change for review.
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

I haven't digested this line by line yet but I agree that what you
describe above would be a good end state.

If I read the patch correctly then we would omit one notification
for the master case. Were both notifications exactly identical
previously?

This has the chance of breaking existing users terribly.

Re: [PATCH net-next] bridge: fix setlink/dellink notifications

[roopa]

On 1/14/15, 11:41 AM, Thomas Graf wrote:
> On 01/13/15 at 10:48pm, roopa@cumulusnetworks.com wrote:
>> 2) Generate one notification from master and the other notification from
>>     self (This seems to be ideal):
>>       For master: the master driver will send notification (bridge in this
>> 	example)
>>       For self: the self driver will send notification (rocker in the above
>> 	example. It can use helpers from rtnetlink.c to do so. Like the
>> 	ndo_dflt_bridge_getlink api).
>>
>> This patch implements 2) (leaving the 'rtnl_bridge_notify' around to be used
>> with 'self').
>>
>> CC'ing others who might be affected by this change for review.
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> I haven't digested this line by line yet but I agree that what you
> describe above would be a good end state.
>
> If I read the patch correctly then we would omit one notification
> for the master case.
yes, correct.
> Were both notifications exactly identical
> previously?
> This has the chance of breaking existing users terribly.

yes AFAICT, because

rtnl_bridge_notify() from rtnetlink.c for bridge  results in calling 
br_fill_ifinfo() via ndo_bridge_getlink()
and the notification from bridge driver br_ifinfo_notify() also ends up 
calling br_fill_ifinfo().

Thanks,
Roopa

RE: [PATCH net-next] bridge: fix setlink/dellink notifications

[Arad, Ronen]

>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>Behalf Of roopa@cumulusnetworks.com
>Sent: Tuesday, January 13, 2015 10:49 PM
>To: netdev@vger.kernel.org; shemminger@vyatta.com; vyasevic@redhat.com;
>john.fastabend@gmail.com; tgraf@suug.ch; jhs@mojatatu.com; sfeldma@gmail.com;
>jiri@resnulli.us
>Cc: wkok@cumulusnetworks.com
>Subject: [PATCH net-next] bridge: fix setlink/dellink notifications
>
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
[..]
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index d06107d..4ac79ff 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -2876,13 +2876,6 @@ static int rtnl_bridge_notify(struct net_device *dev,
>u16 flags)

The 'flags' argument was only used for applying the same handling of
MASTER/SELF flags to notification as used for setlink/delink.
This patch eliminates the MASTER case and leaves only SELF notification.
It seems clearer to eliminate flags argument and rename the function to
something like rtnl_bridge_self_notify().
   
> 		goto errout;
> 	}
>
>-	if ((!flags || (flags & BRIDGE_FLAGS_MASTER)) &&
>-	    br_dev && br_dev->netdev_ops->ndo_bridge_getlink) {
>-		err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
>-		if (err < 0)
>-			goto errout;
>-	}
>-
> 	if ((flags & BRIDGE_FLAGS_SELF) &&
> 	    dev->netdev_ops->ndo_bridge_getlink) {
> 		err = dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
>@@ -2958,16 +2951,19 @@ static int rtnl_bridge_setlink(struct sk_buff *skb,
>struct nlmsghdr *nlh)
> 			err = -EOPNOTSUPP;
> 		else
> 			err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
>-
>-		if (!err)
>+		if (!err) {
> 			flags &= ~BRIDGE_FLAGS_SELF;
>+
>+			/* Generate event to notify upper layer of bridge
>+			 * change
>+			 */
>+			if (!err)
>+				err = rtnl_bridge_notify(dev, oflags);
>+		}
> 	}
>
> 	if (have_flags)
> 		memcpy(nla_data(attr), &flags, sizeof(flags));

What is the purpose of the above two lines (not changed by the patch)?
They seem to copy over the flags with the successfully applied cases
(MASTER and/or SELF) flags cleared back into the incoming netlink message.
I could not figure any place where the modified flags attribute is used

>-	/* Generate event to notify upper layer of bridge change */
>-	if (!err)
>-		err = rtnl_bridge_notify(dev, oflags);
> out:
> 	return err;
> }
>@@ -3032,15 +3028,19 @@ static int rtnl_bridge_dellink(struct sk_buff *skb,
>struct nlmsghdr *nlh)
> 		else
> 			err = dev->netdev_ops->ndo_bridge_dellink(dev, nlh);
>
>-		if (!err)
>+		if (!err) {
> 			flags &= ~BRIDGE_FLAGS_SELF;
>+
>+			/* Generate event to notify upper layer of bridge
>+			 * change
>+			 */
>+			err = rtnl_bridge_notify(dev, oflags);
>+		}
>+
> 	}
>
> 	if (have_flags)
> 		memcpy(nla_data(attr), &flags, sizeof(flags));
>-	/* Generate event to notify upper layer of bridge change */
>-	if (!err)
>-		err = rtnl_bridge_notify(dev, oflags);
> out:
> 	return err;
> }
>--
>1.7.10.4
>
>--
>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 net-next] bridge: fix setlink/dellink notifications

[John Fastabend]

On 01/14/2015 03:22 PM, Arad, Ronen wrote:
>
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>> Behalf Of roopa@cumulusnetworks.com
>> Sent: Tuesday, January 13, 2015 10:49 PM
>> To: netdev@vger.kernel.org; shemminger@vyatta.com; vyasevic@redhat.com;
>> john.fastabend@gmail.com; tgraf@suug.ch; jhs@mojatatu.com; sfeldma@gmail.com;
>> jiri@resnulli.us
>> Cc: wkok@cumulusnetworks.com
>> Subject: [PATCH net-next] bridge: fix setlink/dellink notifications
>>
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
> [..]

[...]

>> 			err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
>> -
>> -		if (!err)
>> +		if (!err) {
>> 			flags &= ~BRIDGE_FLAGS_SELF;
>> +
>> +			/* Generate event to notify upper layer of bridge
>> +			 * change
>> +			 */
>> +			if (!err)
>> +				err = rtnl_bridge_notify(dev, oflags);
>> +		}
>> 	}
>>
>> 	if (have_flags)
>> 		memcpy(nla_data(attr), &flags, sizeof(flags));
>
> What is the purpose of the above two lines (not changed by the patch)?
> They seem to copy over the flags with the successfully applied cases
> (MASTER and/or SELF) flags cleared back into the incoming netlink message.
> I could not figure any place where the modified flags attribute is used

This allows userspace to learn which operation failed when it is an
operation to set both the software bridge via BRIDGE_FLAGS_MASTER and
the the hardware via BRIDGE_FLAGS_SELF. When we get the error back
software looks at the flags to figure out how to recover/retry/etc.

.John

-- 
John Fastabend         Intel Corporation

Re: [PATCH net-next] bridge: fix setlink/dellink notifications

[roopa]

On 1/14/15, 3:36 PM, John Fastabend wrote:
> On 01/14/2015 03:22 PM, Arad, Ronen wrote:
>>
>>
>>> -----Original Message-----
>>> From: netdev-owner@vger.kernel.org 
>>> [mailto:netdev-owner@vger.kernel.org] On
>>> Behalf Of roopa@cumulusnetworks.com
>>> Sent: Tuesday, January 13, 2015 10:49 PM
>>> To: netdev@vger.kernel.org; shemminger@vyatta.com; vyasevic@redhat.com;
>>> john.fastabend@gmail.com; tgraf@suug.ch; jhs@mojatatu.com; 
>>> sfeldma@gmail.com;
>>> jiri@resnulli.us
>>> Cc: wkok@cumulusnetworks.com
>>> Subject: [PATCH net-next] bridge: fix setlink/dellink notifications
>>>
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>> [..]
>
> [...]
>
>>>             err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
>>> -
>>> -        if (!err)
>>> +        if (!err) {
>>>             flags &= ~BRIDGE_FLAGS_SELF;
>>> +
>>> +            /* Generate event to notify upper layer of bridge
>>> +             * change
>>> +             */
>>> +            if (!err)
>>> +                err = rtnl_bridge_notify(dev, oflags);
>>> +        }
>>>     }
>>>
>>>     if (have_flags)
>>>         memcpy(nla_data(attr), &flags, sizeof(flags));
>>
>> What is the purpose of the above two lines (not changed by the patch)?
>> They seem to copy over the flags with the successfully applied cases
>> (MASTER and/or SELF) flags cleared back into the incoming netlink 
>> message.
>> I could not figure any place where the modified flags attribute is used
>
> This allows userspace to learn which operation failed when it is an
> operation to set both the software bridge via BRIDGE_FLAGS_MASTER and
> the the hardware via BRIDGE_FLAGS_SELF. When we get the error back
> software looks at the flags to figure out how to recover/retry/etc.
Ah ok, I was also wondering why that was there,

thanks,
Roopa

Re: [PATCH net-next] bridge: fix setlink/dellink notifications

[tgraf]

On 01/14/15 at 03:36pm, John Fastabend wrote:
> On 01/14/2015 03:22 PM, Arad, Ronen wrote:
> >What is the purpose of the above two lines (not changed by the patch)?
> >They seem to copy over the flags with the successfully applied cases
> >(MASTER and/or SELF) flags cleared back into the incoming netlink message.
> >I could not figure any place where the modified flags attribute is used
> 
> This allows userspace to learn which operation failed when it is an
> operation to set both the software bridge via BRIDGE_FLAGS_MASTER and
> the the hardware via BRIDGE_FLAGS_SELF. When we get the error back
> software looks at the flags to figure out how to recover/retry/etc.

The intent of including the original message in the error Netlink
message was originally to track the request that lead to the error ;-)

Re: [PATCH net-next] bridge: fix setlink/dellink notifications

[roopa]

On 1/14/15, 3:22 PM, Arad, Ronen wrote:
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>> Behalf Of roopa@cumulusnetworks.com
>> Sent: Tuesday, January 13, 2015 10:49 PM
>> To: netdev@vger.kernel.org; shemminger@vyatta.com; vyasevic@redhat.com;
>> john.fastabend@gmail.com; tgraf@suug.ch; jhs@mojatatu.com; sfeldma@gmail.com;
>> jiri@resnulli.us
>> Cc: wkok@cumulusnetworks.com
>> Subject: [PATCH net-next] bridge: fix setlink/dellink notifications
>>
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
> [..]
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index d06107d..4ac79ff 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2876,13 +2876,6 @@ static int rtnl_bridge_notify(struct net_device *dev,
>> u16 flags)
> The 'flags' argument was only used for applying the same handling of
> MASTER/SELF flags to notification as used for setlink/delink.
> This patch eliminates the MASTER case and leaves only SELF notification.
> It seems clearer to eliminate flags argument and rename the function to
> something like rtnl_bridge_self_notify().
sure, if that makes it clearer.

Thanks,
Roopa