Re: [**RFC**] [IPV6]: Support RFC3542 IPV6_PKTINFO socket option.

[Vlad Yasevich <vladislav.yasevich@hp.com>]

Hi Yoshifuji-san

Some more comments below...

YOSHIFUJI Hideaki / 吉藤英明 wrote:
> 
> Thank you.  Here's take 2.
> 
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index b1fe7ac..119363a 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -496,7 +496,58 @@ int datagram_recv_ctl(struct sock *sk, struct msghdr *msg, struct sk_buff *skb)
>  	return 0;
>  }
>  
> -int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
> +int ip6_datagram_set_pktinfo(struct sock *sk,
> +			     struct in6_pktinfo *src_info,
> +			     struct flowi *fl)
> +{
> +	struct net_device *dev = NULL;
> +	int addr_type;
> +	int err = 0;
> +	struct in6_addr *saddr = sk ? &inet6_sk(sk)->saddr : NULL;
> +
> +	if (src_info->ipi6_ifindex) {
> +		dev = dev_get_by_index(src_info->ipi6_ifindex);
> +		if (!dev)
> +			return -ENODEV;
> +	}
> +
> +	fl->oif = src_info->ipi6_ifindex;
> +
> +	addr_type = ipv6_addr_type(&src_info->ipi6_addr);
> +	if (addr_type == IPV6_ADDR_ANY)
> +		goto out;
> +
> +	err = -EINVAL;
> +
> +	if (sk && inet_sk(sk)->is_icsk)
> +		goto out;
> +

> +	if (saddr) {
> +		if (!ipv6_addr_any(saddr))
> +			goto out;
> +		if (!ipv6_addr_equal(&src_info->ipi6_addr, saddr))
> +			goto out;
> +	}

shouldn't the block above go away?  IPV6_PKTINFO can override the
source address that was bound or specified via other options.

 diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index aa3d07c..3ebcef8 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -456,6 +456,28 @@ sticky_done:
>  		break;
>  	}
>  
> +	case IPV6_PKTINFO:
> +	{
> +		struct in6_pktinfo pktinfo;
> +		struct flowi fl;
> +		int err;
> +
> +		if (optlen < sizeof(pktinfo))
> +			return -EINVAL;
> +		if (copy_from_user(&pktinfo, optval, sizeof(pktinfo)))
> +			return -EFAULT;
> +
> +		fl.fl6_flowlabel = 0;
> +		fl.oif = sk->sk_bound_dev_if;
> +
> +		lock_sock(sk);
> +		err = ip6_datagram_set_pktinfo(sk, &pktinfo, &fl);
> +		if (!err)
> +			sk->sk_bound_dev_if = fl.oif;
> +		release_sock(sk);
> +		break;
> +	}
> +

Shouldn't this stash the source address out of the fl into something?
Otherwise we lose the source address information.

It seems like adding to ip6_txoptions might be an option.
We seem to have the same issue for IPV6_2292PKTOPTIONS case too.

>  	case IPV6_2292PKTOPTIONS:
>  	{
>  		struct ipv6_txoptions *opt = NULL;
> @@ -490,7 +512,7 @@ sticky_done:
>  		msg.msg_controllen = optlen;
>  		msg.msg_control = (void*)(opt+1);
>  
> -		retv = datagram_send_ctl(&msg, &fl, opt, &junk, &junk);
> +		retv = datagram_send_ctl(sk, &msg, &fl, opt, &junk, &junk);
>  		if (retv)
>  			goto done;
>  update:
> @@ -933,6 +955,29 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
>  		val = np->ipv6only;
>  		break;
>  
> +	case IPV6_PKTINFO:
> +	{
> +		struct in6_pktinfo pktinfo;
> +
> +		lock_sock(sk);
> +		val = sk->sk_bound_dev_if;
> +		release_sock(sk);
> +
> +		/*
> +		 * XXX: we may need to clear pktinfo to avoid
> +		 * disclosing stack here.
> +		 */
> +		pktinfo.ipi6_addr = inet6_sk(sk)->saddr;
> +		pktinfo.ipi6_ifindex = val;
> +
> +		len = min_t(unsigned int, sizeof(pktinfo), len);
> +		if (put_user(len, optlen))
> +			return -EFAULT;
> +		if (copy_to_user(optval, &pktinfo, len))
> +			return -EFAULT;
> +		return 0;
> +	}
> +

This returns the bound address, not the address specified in the setsockopt.

Thanks
-vlad