Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Maxim Mikityanskiy <maximmi@mellanox.com>
To: Jonas Bonn <jonas@norrbonn.se>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: "David S . Miller" <davem@davemloft.net>,
	Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Subject: Re: [PATCH 1/1] Address regression in inet6_validate_link_af
Date: Wed, 12 Jun 2019 10:42:34 +0000
Message-ID: <58ac6ec1-9255-0e51-981a-195c2b1ac380@mellanox.com> (raw)
In-Reply-To: <20190611100327.16551-1-jonas@norrbonn.se>

On 2019-06-11 13:03, Jonas Bonn wrote:
> Patch 7dc2bccab0ee37ac28096b8fcdc390a679a15841 introduces a regression
> with systemd 241.  In that revision, systemd-networkd fails to pass the
> required flags early enough.  This appears to be addressed in later
> versions of systemd, but for users of version 241 where systemd-networkd
> nonetheless worked with earlier kernels, the strict check introduced by
> the patch causes a regression in behaviour.
> 
> This patch converts the failure to supply the required flags from an
> error into a warning.
The purpose of my patch was to prevent a partial configuration update on 
invalid input. -EINVAL was returned both before and after my patch, the 
difference is that before my patch there was a partial update and a warning.

Your patch basically makes mine pointless, because you revert the fix, 
and now we'll have the same partial update and two warnings.

One more thing is that after applying your patch on top of mine, the 
kernel won't return -EINVAL anymore on invalid input. Returning -EINVAL 
is what happened before my patch, and also after my patch.

Regarding the systemd issue, I don't think we should change the kernel 
to adapt to bugs in systemd. systemd didn't have this bug from day one, 
it was a regression introduced in [1]. The kernel has always returned 
-EINVAL here, but the behavior before my patch was basically a UB, and 
after the patch it's well-defined. If systemd saw EINVAL and relied on 
the UB that came with it, it can't be a reason enough to break the kernel.

Moreover, the bug looks fixed in systemd's master, so what you suggest 
is to insert a kernel-side workaround for an old version of software 
when there is a fixed one.

Please correct me if anything I say is wrong.

Thanks,
Max

[1]: 
https://github.com/systemd/systemd/commit/0e2fdb83bb5e22047e0c7cc058b415d0e93f02cf

> With this, systemd-networkd version 241 once
> again is able to bring up the link, albeit not quite as intended and
> thereby with a warning in the kernel log.
> 
> CC: Maxim Mikityanskiy <maximmi@mellanox.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> CC: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
> ---
>   net/ipv6/addrconf.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 081bb517e40d..e2477bf92e12 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -5696,7 +5696,8 @@ static int inet6_validate_link_af(const struct net_device *dev,
>   		return err;
>   
>   	if (!tb[IFLA_INET6_TOKEN] && !tb[IFLA_INET6_ADDR_GEN_MODE])
> -		return -EINVAL;
> +		net_warn_ratelimited(
> +			"required link flag omitted: TOKEN/ADDR_GEN_MODE\n");
>   
>   	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
>   		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
> 


  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-11 10:03 Jonas Bonn
2019-06-12 10:42 ` Maxim Mikityanskiy [this message]
2019-06-13  6:45   ` Jonas Bonn
2019-06-13 14:13     ` Maxim Mikityanskiy
2019-06-14  8:20       ` Jonas Bonn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=58ac6ec1-9255-0e51-981a-195c2b1ac380@mellanox.com \
    --to=maximmi@mellanox.com \
    --cc=davem@davemloft.net \
    --cc=jonas@norrbonn.se \
    --cc=kuznet@ms2.inr.ac.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=yoshfuji@linux-ipv6.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git