[RFC] inet6_validate_link_af improvements

[RFC] inet6_validate_link_af improvements

[Maxim Mikityanskiy]

A recent bug in systemd [1] triggered the following kernel warning:

  A link change request failed with some changes committed already.
  Interface eth1 may have been left with an inconsistent configuration,
  please check.

do_setlink() performs multiple configuration updates, and if any of them
fails, do_setlink() has no way to revert the previous ones. It is also
not easy to validate everything in advance and perform a non-failing
update then. However, do_setlink() has some basic validation that can be
extended at least this case. IMO, it's better to fail before doing any
changes than to perform a partial configuration update.

This RFC contains two patches that move some checks to the validation
stage (inet6_validate_link_af() function). Only one of the patches (if
any) should be applied. Patch 1 only checks the presence of at least one
parameter, and patch 2 also moves the validation for addrgenmode that is
currently part of inet6_set_link_af(). Of course, there are still many
ways how do_setlink() can fail and perform a partial update, but IMO
it's better to prevent at least some cases that we can.

Please express your opinions on this fix: do we need it, do we want to
validate as much as possible (patch 2) or only basic stuff like the
presence of parameters (patch 1)? I'm looking forward to hearing the
feedback.

Thanks,
Max

[1]: https://github.com/systemd/systemd/issues/12504

-- 
2.19.1

[RFC 1] Validate required parameters in inet6_validate_link_af

[Maxim Mikityanskiy]

inet6_set_link_af requires that at least one of IFLA_INET6_TOKEN or
IFLA_INET6_ADDR_GET_MODE is passed. If none of them is passed, it
returns -EINVAL, which may cause do_setlink() to fail in the middle of
processing other commands and give the following warning message:

  A link change request failed with some changes committed already.
  Interface eth0 may have been left with an inconsistent configuration,
  please check.

Check the presence of at least one of them in inet6_validate_link_af to
detect invalid parameters at an early stage, before do_setlink does
anything.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/ipv6/addrconf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f96d1de79509..ef38d381ccbd 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5665,12 +5665,20 @@ static int inet6_validate_link_af(const struct net_device *dev,
 				  const struct nlattr *nla)
 {
 	struct nlattr *tb[IFLA_INET6_MAX + 1];
+	int err;
 
 	if (dev && !__in6_dev_get(dev))
 		return -EAFNOSUPPORT;
 
-	return nla_parse_nested_deprecated(tb, IFLA_INET6_MAX, nla,
-					   inet6_af_policy, NULL);
+	err = nla_parse_nested_deprecated(tb, IFLA_INET6_MAX, nla,
+					  inet6_af_policy, NULL);
+	if (err)
+		return err;
+
+	if (!tb[IFLA_INET6_TOKEN] && !tb[IFLA_INET6_ADDR_GEN_MODE])
+		return -EINVAL;
+
+	return 0;
 }
 
 static int check_addr_gen_mode(int mode)
-- 
2.19.1

[RFC 2] Validate required parameters in inet6_validate_link_af

[Maxim Mikityanskiy]

inet6_set_link_af requires that at least one of IFLA_INET6_TOKEN or
IFLA_INET6_ADDR_GET_MODE is passed. If none of them is passed, it
returns -EINVAL, which may cause do_setlink() to fail in the middle of
processing other commands and give the following warning message:

  A link change request failed with some changes committed already.
  Interface eth0 may have been left with an inconsistent configuration,
  please check.

Check the presence of at least one of them in inet6_validate_link_af to
detect invalid parameters at an early stage, before do_setlink does
anything. Also validate the address generation mode at an early stage.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/ipv6/addrconf.c | 61 +++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f96d1de79509..904bd6f5472f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5661,18 +5661,6 @@ static const struct nla_policy inet6_af_policy[IFLA_INET6_MAX + 1] = {
 	[IFLA_INET6_TOKEN]		= { .len = sizeof(struct in6_addr) },
 };
 
-static int inet6_validate_link_af(const struct net_device *dev,
-				  const struct nlattr *nla)
-{
-	struct nlattr *tb[IFLA_INET6_MAX + 1];
-
-	if (dev && !__in6_dev_get(dev))
-		return -EAFNOSUPPORT;
-
-	return nla_parse_nested_deprecated(tb, IFLA_INET6_MAX, nla,
-					   inet6_af_policy, NULL);
-}
-
 static int check_addr_gen_mode(int mode)
 {
 	if (mode != IN6_ADDR_GEN_MODE_EUI64 &&
@@ -5693,14 +5681,48 @@ static int check_stable_privacy(struct inet6_dev *idev, struct net *net,
 	return 1;
 }
 
+static int inet6_validate_link_af(const struct net_device *dev,
+				  const struct nlattr *nla)
+{
+	struct inet6_dev *idev = NULL;
+	struct nlattr *tb[IFLA_INET6_MAX + 1];
+	int err;
+
+	if (dev) {
+		idev = __in6_dev_get(dev);
+		if (!idev)
+			return -EAFNOSUPPORT;
+	}
+
+	err = nla_parse_nested_deprecated(tb, IFLA_INET6_MAX, nla,
+					  inet6_af_policy, NULL);
+	if (err)
+		return err;
+
+	err = -EINVAL;
+
+	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
+		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
+
+		if (check_addr_gen_mode(mode) < 0)
+			return -EINVAL;
+		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
+			return -EINVAL;
+
+		err = 0;
+	}
+
+	if (tb[IFLA_INET6_TOKEN])
+		err = 0;
+
+	return err;
+}
+
 static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
 {
-	int err = -EINVAL;
 	struct inet6_dev *idev = __in6_dev_get(dev);
 	struct nlattr *tb[IFLA_INET6_MAX + 1];
-
-	if (!idev)
-		return -EAFNOSUPPORT;
+	int err;
 
 	if (nla_parse_nested_deprecated(tb, IFLA_INET6_MAX, nla, NULL, NULL) < 0)
 		BUG();
@@ -5714,15 +5736,10 @@ static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
 	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
 		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
 
-		if (check_addr_gen_mode(mode) < 0 ||
-		    check_stable_privacy(idev, dev_net(dev), mode) < 0)
-			return -EINVAL;
-
 		idev->cnf.addr_gen_mode = mode;
-		err = 0;
 	}
 
-	return err;
+	return 0;
 }
 
 static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
-- 
2.19.1

Re: [RFC] inet6_validate_link_af improvements

[David Miller]

From: Maxim Mikityanskiy <maximmi@mellanox.com>
Date: Mon, 13 May 2019 15:05:28 +0000

> A recent bug in systemd [1] triggered the following kernel warning:
> 
>   A link change request failed with some changes committed already.
>   Interface eth1 may have been left with an inconsistent configuration,
>   please check.
> 
> do_setlink() performs multiple configuration updates, and if any of them
> fails, do_setlink() has no way to revert the previous ones. It is also
> not easy to validate everything in advance and perform a non-failing
> update then. However, do_setlink() has some basic validation that can be
> extended at least this case. IMO, it's better to fail before doing any
> changes than to perform a partial configuration update.
> 
> This RFC contains two patches that move some checks to the validation
> stage (inet6_validate_link_af() function). Only one of the patches (if
> any) should be applied. Patch 1 only checks the presence of at least one
> parameter, and patch 2 also moves the validation for addrgenmode that is
> currently part of inet6_set_link_af(). Of course, there are still many
> ways how do_setlink() can fail and perform a partial update, but IMO
> it's better to prevent at least some cases that we can.
> 
> Please express your opinions on this fix: do we need it, do we want to
> validate as much as possible (patch 2) or only basic stuff like the
> presence of parameters (patch 1)? I'm looking forward to hearing the
> feedback.

I think your patches should go in now.

And longer term for the other cases we should move to a prepare-->commit
model so that resources can be allocated in one pass and only commit the
result if all resources can be obtained together.

Re: [RFC 2] Validate required parameters in inet6_validate_link_af

[Jakub Kicinski]

On Mon, 13 May 2019 15:05:30 +0000, Maxim Mikityanskiy wrote:
> +	err = -EINVAL;
> +
> +	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
> +		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
> +
> +		if (check_addr_gen_mode(mode) < 0)
> +			return -EINVAL;
> +		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
> +			return -EINVAL;
> +
> +		err = 0;
> +	}
> +
> +	if (tb[IFLA_INET6_TOKEN])
> +		err = 0;
> +
> +	return err;

While at it could you forgo the retval optimization?  Most of the time
it just leads to less readable code for no gain.

The normal way to write this code would be:

	if (!tb[IFLA_INET6_ADDR_GEN_MODE] && !tb[IFLA_INET6_TOKEN])
		return -EINVAL;

	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);

		if (check_addr_gen_mode(mode) < 0)
			return -EINVAL;
		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
			return -EINVAL;
	}

	return 0;

Re: [RFC 2] Validate required parameters in inet6_validate_link_af

[Maxim Mikityanskiy]

On 2019-05-15 02:32, Jakub Kicinski wrote:
> On Mon, 13 May 2019 15:05:30 +0000, Maxim Mikityanskiy wrote:
>> +	err = -EINVAL;
>> +
>> +	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
>> +		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
>> +
>> +		if (check_addr_gen_mode(mode) < 0)
>> +			return -EINVAL;
>> +		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
>> +			return -EINVAL;
>> +
>> +		err = 0;
>> +	}
>> +
>> +	if (tb[IFLA_INET6_TOKEN])
>> +		err = 0;
>> +
>> +	return err;
> 
> While at it could you forgo the retval optimization?  Most of the time
> it just leads to less readable code for no gain.

OK, I'll make this change in a respin.

> The normal way to write this code would be:
> 
> 	if (!tb[IFLA_INET6_ADDR_GEN_MODE] && !tb[IFLA_INET6_TOKEN])
> 		return -EINVAL;

Yeah, that's how I wrote this check in RFC 1, but here in this patch I 
decided to preserve the pattern that was used in inet6_set_link_af 
before my change, to minimize the changes. I agree it's less readable (I 
didn't like the error handling flow in inet6_set_link_af either), so 
I'll fix it. Thanks for reviewing!

> 	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
> 		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
> 
> 		if (check_addr_gen_mode(mode) < 0)
> 			return -EINVAL;
> 		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
> 			return -EINVAL;
> 	}
> 
> 	return 0;
>