[PATCH net-next v4 0/3] bridge FDB ageing time.

[PATCH net-next v4 0/3] bridge FDB ageing time.

[Siva Mannem]

v4:
Changes based on review comments.
  - Validate all netlink attributes and return error if any of the    
    validation fails.
v3:
Changes based on review comments.
  - Ensure that the newly configured FDB ageing time becomes effective
    immediately and also make the behavior consistent when setting ageing_time
    using any of the existing mechanisms(sysfs, ioctl, netlink).
v2:
  - Added commit message.
  - Fix style problems reported by checkpatch.pl.

v1:
  - Configure bridge FDB ageing time using netlink.

Siva Mannem (3):
  Configure bridge FDB ageing time using netlink.
  
  Ensure that the newly configured FDB ageing time becomes effective
  immediately. It also makes the behavior consistent when the ageing_timer is
  set using any of the three existing mechanisms(sysfs, ioctl, netlink).
  
  Validate all netlink attributes and return error if any of the    
  validation fails.

 include/uapi/linux/if_link.h |  1 +
 net/bridge/br_device.c       | 17 ++++++++++++++
 net/bridge/br_ioctl.c        |  3 +--
 net/bridge/br_netlink.c      | 53 +++++++++++++++++++++++++++++++++++++++++---
 net/bridge/br_private.h      |  5 +++++
 net/bridge/br_sysfs_br.c     |  8 +------
 6 files changed, 75 insertions(+), 12 deletions(-)

-- 
2.1.0

[PATCH net-next v4 1/3] Configure bridge FDB ageing time using netlink.

[Siva Mannem]

This patch allows user to configure bridge's FDB ageing using
netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
as per ieee8021QBridgeFdbAgingTime.

Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
---
 include/uapi/linux/if_link.h |  1 +
 net/bridge/br_device.c       | 13 +++++++++++++
 net/bridge/br_netlink.c      | 14 ++++++++++++--
 net/bridge/br_private.h      |  5 +++++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 756436e..4a59232 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -224,6 +224,7 @@ enum {
 	IFLA_BR_FORWARD_DELAY,
 	IFLA_BR_HELLO_TIME,
 	IFLA_BR_MAX_AGE,
+	IFLA_BR_AGEING_TIME,
 	__IFLA_BR_MAX,
 };
 
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 294cbcc..1a665aa 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -392,3 +392,16 @@ void br_dev_setup(struct net_device *dev)
 	br_stp_timer_init(br);
 	br_multicast_init(br);
 }
+
+int br_set_ageing_time(struct net_bridge *br, unsigned long val)
+{
+	unsigned long t = clock_t_to_jiffies(val);
+
+	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
+		return -ERANGE;
+
+	spin_lock_bh(&br->lock);
+	br->ageing_time = t;
+	spin_unlock_bh(&br->lock);
+	return 0;
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 8bc6b67..d80e802 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -733,6 +733,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
 	[IFLA_BR_FORWARD_DELAY]	= { .type = NLA_U32 },
 	[IFLA_BR_HELLO_TIME]	= { .type = NLA_U32 },
 	[IFLA_BR_MAX_AGE]	= { .type = NLA_U32 },
+	[IFLA_BR_AGEING_TIME]	= { .type = NLA_U32 },
 };
 
 static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
@@ -762,6 +763,12 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 			return err;
 	}
 
+	if (data[IFLA_BR_AGEING_TIME]) {
+		err = br_set_ageing_time(br, nla_get_u32(data[IFLA_BR_AGEING_TIME]));
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -770,6 +777,7 @@ static size_t br_get_size(const struct net_device *brdev)
 	return nla_total_size(sizeof(u32)) +	/* IFLA_BR_FORWARD_DELAY  */
 	       nla_total_size(sizeof(u32)) +	/* IFLA_BR_HELLO_TIME */
 	       nla_total_size(sizeof(u32)) +	/* IFLA_BR_MAX_AGE */
+	       nla_total_size(sizeof(u32)) +	/* IFLA_BR_AGEING_TIME */
 	       0;
 }
 
@@ -778,11 +786,13 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 	struct net_bridge *br = netdev_priv(brdev);
 	u32 forward_delay = jiffies_to_clock_t(br->forward_delay);
 	u32 hello_time = jiffies_to_clock_t(br->hello_time);
-	u32 age_time = jiffies_to_clock_t(br->max_age);
+	u32 max_age = jiffies_to_clock_t(br->max_age);
+	u32 ageing_time = jiffies_to_clock_t(br->ageing_time);
 
 	if (nla_put_u32(skb, IFLA_BR_FORWARD_DELAY, forward_delay) ||
 	    nla_put_u32(skb, IFLA_BR_HELLO_TIME, hello_time) ||
-	    nla_put_u32(skb, IFLA_BR_MAX_AGE, age_time))
+	    nla_put_u32(skb, IFLA_BR_MAX_AGE, max_age) ||
+	    nla_put_u32(skb, IFLA_BR_AGEING_TIME, ageing_time))
 		return -EMSGSIZE;
 
 	return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f0a0438..325883c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -25,6 +25,10 @@
 
 #define BR_HOLD_TIME (1*HZ)
 
+/* values as per ieee8021QBridgeFdbAgingTime */
+#define BR_MIN_AGEING_TIME  (10 * HZ)
+#define BR_MAX_AGEING_TIME  (1000000 * HZ)
+
 #define BR_PORT_BITS	10
 #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
 #define BR_VLAN_BITMAP_LEN	BITS_TO_LONGS(VLAN_N_VID)
@@ -345,6 +349,7 @@ static inline int br_is_root_bridge(const struct net_bridge *br)
 void br_dev_setup(struct net_device *dev);
 void br_dev_delete(struct net_device *dev, struct list_head *list);
 netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev);
+int br_set_ageing_time(struct net_bridge *br, unsigned long val);
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
 				       struct sk_buff *skb)
-- 
2.1.0

[PATCH net-next v4 2/3] newly configured FDB ageing time becomes effective immediately

[Siva Mannem]

This patch ensures that the newly configured FDB ageing time becomes effective
immediately. It also makes the behavior consistent when the ageing_timer is set 
using any of the three existing mechanisms(sysfs, ioctl, netlink).

Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
Suggested-by: Scott Feldman <sfeldma@gmail.com>
---
 net/bridge/br_device.c   | 4 ++++
 net/bridge/br_ioctl.c    | 3 +--
 net/bridge/br_sysfs_br.c | 8 +-------
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 1a665aa..293a113 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -396,12 +396,16 @@ void br_dev_setup(struct net_device *dev)
 int br_set_ageing_time(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
+	unsigned long old_ageing_time;
 
 	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
 		return -ERANGE;
 
 	spin_lock_bh(&br->lock);
+	old_ageing_time =  br->ageing_time;
 	br->ageing_time = t;
+	if (br->ageing_time < old_ageing_time)
+		mod_timer(&br->gc_timer, jiffies);
 	spin_unlock_bh(&br->lock);
 	return 0;
 }
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index a9a4a1b..4f836e2 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		br->ageing_time = clock_t_to_jiffies(args[1]);
-		return 0;
+		return br_set_ageing_time(br, args[1]);
 
 	case BRCTL_GET_PORT_INFO:
 	{
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 4c97fc5..1fc2857 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -100,17 +100,11 @@ static ssize_t ageing_time_show(struct device *d,
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->ageing_time));
 }
 
-static int set_ageing_time(struct net_bridge *br, unsigned long val)
-{
-	br->ageing_time = clock_t_to_jiffies(val);
-	return 0;
-}
-
 static ssize_t ageing_time_store(struct device *d,
 				 struct device_attribute *attr,
 				 const char *buf, size_t len)
 {
-	return store_bridge_parm(d, buf, len, set_ageing_time);
+	return store_bridge_parm(d, buf, len, br_set_ageing_time);
 }
 static DEVICE_ATTR_RW(ageing_time);
 
-- 
2.1.0

[PATCH net-next v4 3/3] Validate all netlink attributes and return error if any of the validation fails.

[Siva Mannem]

This patch validates all netlink attributes and return error if any of the
validation fails.

Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
Suggested-by: David Miller <davem@davemloft.net>
---
 net/bridge/br_netlink.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index d80e802..0b18c0d 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -740,12 +740,49 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 			 struct nlattr *data[])
 {
 	struct net_bridge *br = netdev_priv(brdev);
-	int err;
+	unsigned long forward_delay;
+	unsigned long hello_time;
+	unsigned long max_age;
+	unsigned long ageing_time;
+	u32 t;
+	int err = -ERANGE;
 
 	if (!data)
 		return 0;
 
 	if (data[IFLA_BR_FORWARD_DELAY]) {
+		t = nla_get_u32(data[IFLA_BR_FORWARD_DELAY]);
+		forward_delay = clock_t_to_jiffies(t);
+		if (forward_delay < BR_MIN_FORWARD_DELAY ||
+		    forward_delay > BR_MAX_FORWARD_DELAY)
+			return err;
+	}
+
+	if (data[IFLA_BR_HELLO_TIME]) {
+		t = nla_get_u32(data[IFLA_BR_HELLO_TIME]);
+		hello_time = clock_t_to_jiffies(t);
+		if (hello_time < BR_MIN_HELLO_TIME ||
+		    hello_time > BR_MAX_HELLO_TIME)
+			return err;
+	}
+
+	if (data[IFLA_BR_MAX_AGE]) {
+		t = nla_get_u32(data[IFLA_BR_MAX_AGE]);
+		max_age = clock_t_to_jiffies(t);
+		if (max_age < BR_MIN_MAX_AGE ||
+		    max_age > BR_MAX_MAX_AGE)
+			return err;
+	}
+
+	if (data[IFLA_BR_AGEING_TIME]) {
+		t = nla_get_u32(data[IFLA_BR_AGEING_TIME]);
+		ageing_time = clock_t_to_jiffies(t);
+		if (ageing_time < BR_MIN_AGEING_TIME ||
+		    ageing_time > BR_MAX_AGEING_TIME)
+			return err;
+	}
+
+	if (data[IFLA_BR_FORWARD_DELAY]) {
 		err = br_set_forward_delay(br, nla_get_u32(data[IFLA_BR_FORWARD_DELAY]));
 		if (err)
 			return err;
-- 
2.1.0

Re: [PATCH net-next v4 3/3] Validate all netlink attributes and return error if any of the validation fails.

[Scott Feldman]

On Mon, Mar 16, 2015 at 4:02 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
> This patch validates all netlink attributes and return error if any of the
> validation fails.
>
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
> Suggested-by: David Miller <davem@davemloft.net>
> ---
>  net/bridge/br_netlink.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index d80e802..0b18c0d 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -740,12 +740,49 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
>                          struct nlattr *data[])
>  {
>         struct net_bridge *br = netdev_priv(brdev);
> -       int err;
> +       unsigned long forward_delay;
> +       unsigned long hello_time;
> +       unsigned long max_age;
> +       unsigned long ageing_time;
> +       u32 t;
> +       int err = -ERANGE;
>
>         if (!data)
>                 return 0;
>
>         if (data[IFLA_BR_FORWARD_DELAY]) {
> +               t = nla_get_u32(data[IFLA_BR_FORWARD_DELAY]);
> +               forward_delay = clock_t_to_jiffies(t);
> +               if (forward_delay < BR_MIN_FORWARD_DELAY ||
> +                   forward_delay > BR_MAX_FORWARD_DELAY)
> +                       return err;

Is there a way to avoiding duplicate range checking code by passing an
extra arg to br_set_forward_delay(..., bool validate_only) and
friends?

Re: [PATCH net-next v4 3/3] Validate all netlink attributes and return error if any of the validation fails.

[David Miller]

From: Siva Mannem <siva.mannem.lnx@gmail.com>
Date: Tue, 17 Mar 2015 04:32:44 +0530

> This patch validates all netlink attributes and return error if any of the
> validation fails.
> 
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
> Suggested-by: David Miller <davem@davemloft.net>

You have to remove the range checking code from all the other
functions too.

Then no errors can be returned by them at all actually and that's an
really nice cleanup.

Please don't make microscopic narrow changes like this, think about
the higher level implications and the things that are no longer
necessary as a result.

Thanks.

Re: [PATCH net-next v4 3/3] Validate all netlink attributes and return error if any of the validation fails.

[David Miller]

Also, this should be the first change in your patch series.

Your current first patch just adds more bogus range checking to the
helper functions, which this patch makes %100 unnecessary.

Re: [PATCH net-next v4 3/3] Validate all netlink attributes and return error if any of the validation fails.

[Siva Mannem]

On Tue, Mar 17, 2015 at 9:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Siva Mannem <siva.mannem.lnx@gmail.com>
> Date: Tue, 17 Mar 2015 04:32:44 +0530
>
>> This patch validates all netlink attributes and return error if any of the
>> validation fails.
>>
>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>> Suggested-by: David Miller <davem@davemloft.net>
>
> You have to remove the range checking code from all the other
> functions too.
>
> Then no errors can be returned by them at all actually and that's an
> really nice cleanup.
>
> Please don't make microscopic narrow changes like this, think about
> the higher level implications and the things that are no longer
> necessary as a result.
>
> Thanks.

These functions(br_set_forward_delay/hello_time/max_age/ageing_time)
are also called from other places i.e old_dev_ioctl() and sysfs code. If
range checking has to be removed from these functions, then the same
has to be added in the path from where they are being invoked.

As Scott mentioned,  these functions can be enhanced to take
extra argument to avoid extra range checking happening in br_changelink()
path. Is this approach ok?
-- 
Regards,
Siva Mannem.

Re: [PATCH net-next v4 3/3] Validate all netlink attributes and return error if any of the validation fails.

[David Miller]

From: Siva Mannem <siva.mannem.lnx@gmail.com>
Date: Tue, 17 Mar 2015 10:29:28 +0530

> As Scott mentioned,  these functions can be enhanced to take
> extra argument to avoid extra range checking happening in br_changelink()
> path. Is this approach ok?

Make validation helper inline functions, call them from the individual
sysfs call sites and from br_changelink().

Re: [PATCH net-next v4 2/3] newly configured FDB ageing time becomes effective immediately

[Sergei Shtylyov]

Hello.

On 3/17/2015 2:02 AM, Siva Mannem wrote:

> This patch ensures that the newly configured FDB ageing time becomes effective
> immediately. It also makes the behavior consistent when the ageing_timer is set
> using any of the three existing mechanisms(sysfs, ioctl, netlink).

> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
> Suggested-by: Scott Feldman <sfeldma@gmail.com>
> ---
>   net/bridge/br_device.c   | 4 ++++
>   net/bridge/br_ioctl.c    | 3 +--
>   net/bridge/br_sysfs_br.c | 8 +-------
>   3 files changed, 6 insertions(+), 9 deletions(-)

> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 1a665aa..293a113 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -396,12 +396,16 @@ void br_dev_setup(struct net_device *dev)
>   int br_set_ageing_time(struct net_bridge *br, unsigned long val)
>   {
>   	unsigned long t = clock_t_to_jiffies(val);
> +	unsigned long old_ageing_time;
>
>   	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>   		return -ERANGE;
>
>   	spin_lock_bh(&br->lock);
> +	old_ageing_time =  br->ageing_time;

    One space is enough after '='...

>   	br->ageing_time = t;
> +	if (br->ageing_time < old_ageing_time)

    Isn't it enough to compare just 't' instead of 'br->ageing_time'?

> +		mod_timer(&br->gc_timer, jiffies);
>   	spin_unlock_bh(&br->lock);
>   	return 0;
>   }
[...]

WBR, Sergei

Re: [PATCH net-next v4 2/3] newly configured FDB ageing time becomes effective immediately

[Siva Mannem]

On Tue, Mar 17, 2015 at 8:11 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 3/17/2015 2:02 AM, Siva Mannem wrote:
>
>> This patch ensures that the newly configured FDB ageing time becomes
>> effective
>> immediately. It also makes the behavior consistent when the ageing_timer
>> is set
>> using any of the three existing mechanisms(sysfs, ioctl, netlink).
>
>
>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>> Suggested-by: Scott Feldman <sfeldma@gmail.com>
>> ---
>>   net/bridge/br_device.c   | 4 ++++
>>   net/bridge/br_ioctl.c    | 3 +--
>>   net/bridge/br_sysfs_br.c | 8 +-------
>>   3 files changed, 6 insertions(+), 9 deletions(-)
>
>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 1a665aa..293a113 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -396,12 +396,16 @@ void br_dev_setup(struct net_device *dev)
>>   int br_set_ageing_time(struct net_bridge *br, unsigned long val)
>>   {
>>         unsigned long t = clock_t_to_jiffies(val);
>> +       unsigned long old_ageing_time;
>>
>>         if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>>                 return -ERANGE;
>>
>>         spin_lock_bh(&br->lock);
>> +       old_ageing_time =  br->ageing_time;
>
>
>    One space is enough after '='...
>
>>         br->ageing_time = t;
>> +       if (br->ageing_time < old_ageing_time)
>
>
>    Isn't it enough to compare just 't' instead of 'br->ageing_time'?
>
>> +               mod_timer(&br->gc_timer, jiffies);
>>         spin_unlock_bh(&br->lock);
>>         return 0;
>>   }
>
> [...]
>
> WBR, Sergei
>
Yep. Will address them in next version.
Thanks.

-- 
Regards,
Siva Mannem.