netdev - Re: [PATCH v2 net-next 1/1] net: Add new protocol attribute to IP addresses

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dffe2ca3-b3f1-e742-f239-9724cf0c1e83@gmail.com>
Date:   Thu, 10 Feb 2022 09:47:24 -0800
From:   David Ahern <dsahern@...il.com>
To:     Jacques de Laval <Jacques.De.Laval@...termo.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 1/1] net: Add new protocol attribute to IP
 addresses

On 2/9/22 8:34 AM, Jacques de Laval wrote:
> diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
> index dfcf3ce0097f..2c487a9de658 100644
> --- a/include/uapi/linux/if_addr.h
> +++ b/include/uapi/linux/if_addr.h
> @@ -35,6 +35,7 @@ enum {
>  	IFA_FLAGS,
>  	IFA_RT_PRIORITY,  /* u32, priority/metric for prefix route */
>  	IFA_TARGET_NETNSID,
> +	IFA_PROTO,

Good to self-document attributes for userspace. See IFA_RT_PRIORITY above.


>  	__IFA_MAX,
>  };
>  
> @@ -69,4 +70,12 @@ struct ifa_cacheinfo {
>  #define IFA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifaddrmsg))
>  #endif
>  
> +/* ifa_protocol */
> +#define IFAPROT_UNSPEC		0
> +#define IFAPROT_KERNEL_LO	1       /* loopback */
> +#define IFAPROT_KERNEL_RA	2       /* auto assigned by kernel from router
> +					   announcement
> +					 */

comment style should be:
/* auto assigned by kernel from router
 * announcement
 */

but I think the comment can be shortened to:
/* set by kernel from router announcement */


> +#define IFAPROT_KERNEL_LL	3       /* link-local set by kernel*/

space between kernel and '*'

> +
>  #endif
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 4744c7839de5..4c72ccd72ceb 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -102,6 +102,7 @@ static const struct nla_policy ifa_ipv4_policy[IFA_MAX+1] = {
>  	[IFA_FLAGS]		= { .type = NLA_U32 },
>  	[IFA_RT_PRIORITY]	= { .type = NLA_U32 },
>  	[IFA_TARGET_NETNSID]	= { .type = NLA_S32 },
> +	[IFA_PROTO]		= { .type = NLA_U8 },
>  };
>  
>  struct inet_fill_args {
> @@ -577,8 +578,10 @@ static int inet_set_ifa(struct net_device *dev, struct in_ifaddr *ifa)
>  		in_dev_hold(in_dev);
>  		ifa->ifa_dev = in_dev;
>  	}
> -	if (ipv4_is_loopback(ifa->ifa_local))
> +	if (ipv4_is_loopback(ifa->ifa_local)) {
>  		ifa->ifa_scope = RT_SCOPE_HOST;
> +		ifa->ifa_proto = IFAPROT_KERNEL_LO;

inet_set_ifa is used by the old ioctl interface. Even if this is a
loopback address, there is no guarantee this particular assignment is
done by the kernel so the ifa_proto should not be set here.

> +	}
>  	return inet_insert_ifa(ifa);
>  }
>  
> @@ -887,6 +890,9 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,
>  	if (tb[IFA_RT_PRIORITY])
>  		ifa->ifa_rt_priority = nla_get_u32(tb[IFA_RT_PRIORITY]);
>  
> +	if (tb[IFA_PROTO])
> +		ifa->ifa_proto = nla_get_u8(tb[IFA_PROTO]);
> +
>  	if (tb[IFA_CACHEINFO]) {
>  		struct ifa_cacheinfo *ci;
>  
> @@ -1623,6 +1629,7 @@ static size_t inet_nlmsg_size(void)
>  	       + nla_total_size(4) /* IFA_BROADCAST */
>  	       + nla_total_size(IFNAMSIZ) /* IFA_LABEL */
>  	       + nla_total_size(4)  /* IFA_FLAGS */
> +	       + nla_total_size(1)  /* IFA_PROTO */
>  	       + nla_total_size(4)  /* IFA_RT_PRIORITY */
>  	       + nla_total_size(sizeof(struct ifa_cacheinfo)); /* IFA_CACHEINFO */
>  }
> @@ -1697,6 +1704,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa,
>  	     nla_put_in_addr(skb, IFA_BROADCAST, ifa->ifa_broadcast)) ||
>  	    (ifa->ifa_label[0] &&
>  	     nla_put_string(skb, IFA_LABEL, ifa->ifa_label)) ||
> +	    nla_put_u8(skb, IFA_PROTO, ifa->ifa_proto) ||

if (ifa->ifa_proto && nla_put_u8(skb, IFA_PROTO, ifa->ifa_proto

only set attribute if protocol is set.


>  	    nla_put_u32(skb, IFA_FLAGS, ifa->ifa_flags) ||
>  	    (ifa->ifa_rt_priority &&
>  	     nla_put_u32(skb, IFA_RT_PRIORITY, ifa->ifa_rt_priority)) ||



> @@ -4946,6 +4956,7 @@ static inline int inet6_ifaddr_msgsize(void)
>  	       + nla_total_size(16) /* IFA_ADDRESS */
>  	       + nla_total_size(sizeof(struct ifa_cacheinfo))
>  	       + nla_total_size(4)  /* IFA_FLAGS */
> +	       + nla_total_size(1)  /* IFA_PROTO */
>  	       + nla_total_size(4)  /* IFA_RT_PRIORITY */;
>  }
>  
> @@ -5023,6 +5034,9 @@ static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
>  	if (nla_put_u32(skb, IFA_FLAGS, ifa->flags) < 0)
>  		goto error;
>  
> +	if (nla_put_u8(skb, IFA_PROTO, ifa->ifa_proto))

only add attribute if proto set.

> +		goto error;
> +
>  	nlmsg_end(skb, nlh);
>  	return 0;
>