[PATCH RFC] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt

[PATCH RFC] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt

[Florent Fourcot]

It is already possible to set/put/renew a label
with IPV6_FLOWLABEL_MGR and setsockopt. This patch
add the possibility to get information about this
label (current value, time before expiration, etc).

It helps application to take decision for a renew
or a release of the label.

Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
---
 include/net/ipv6.h       |  1 +
 net/ipv6/ip6_flowlabel.c | 23 +++++++++++++++++++++++
 net/ipv6/ipv6_sockglue.c | 23 +++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index dd96638..2a5f668 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -250,6 +250,7 @@ struct ipv6_txoptions *fl6_merge_options(struct ipv6_txoptions *opt_space,
 					 struct ipv6_txoptions *fopt);
 void fl6_free_socklist(struct sock *sk);
 int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen);
+int ipv6_flowlabel_opt_get(struct sock *sk, struct in6_flowlabel_req *freq);
 int ip6_flowlabel_init(void);
 void ip6_flowlabel_cleanup(void);
 
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index 819578e..34f58b7 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -475,6 +475,29 @@ static inline void fl_link(struct ipv6_pinfo *np, struct ipv6_fl_socklist *sfl,
 	spin_unlock_bh(&ip6_sk_fl_lock);
 }
 
+int ipv6_flowlabel_opt_get(struct sock *sk, struct in6_flowlabel_req *freq)
+{
+	struct ipv6_pinfo *np = inet6_sk(sk);
+	struct ipv6_fl_socklist *sfl;
+
+	rcu_read_lock_bh();
+	for_each_sk_fl_rcu(np, sfl) {
+		if (sfl->fl->label == (np->flow_label & IPV6_FLOWLABEL_MASK)) {
+			freq->flr_label = sfl->fl->label;
+			freq->flr_dst = sfl->fl->dst;
+			freq->flr_share = sfl->fl->share;
+			freq->flr_expires = (sfl->fl->expires - jiffies) / HZ;
+			freq->flr_linger = sfl->fl->linger / HZ;
+
+			rcu_read_unlock_bh();
+			return 0;
+		}
+	}
+	rcu_read_unlock_bh();
+
+	return 0;
+}
+
 int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen)
 {
 	int uninitialized_var(err);
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4919a8e..9ca96c2 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1212,6 +1212,29 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
 		val = np->sndflow;
 		break;
 
+	case IPV6_FLOWLABEL_MGR:
+	{
+		struct in6_flowlabel_req freq;
+
+		if (len < sizeof(freq))
+			return -EINVAL;
+
+		len = sizeof(freq);
+		memset(&freq, 0, sizeof(freq));
+
+		val = ipv6_flowlabel_opt_get(sk, &freq);
+		if (val < 0)
+			return val;
+
+		if (put_user(len, optlen))
+			return -EFAULT;
+		if (copy_to_user(optval, &freq, len))
+			return -EFAULT;
+
+		return 0;
+		break;
+	}
+
 	case IPV6_ADDR_PREFERENCES:
 		val = 0;
 
-- 
1.8.4.rc3

Re: [PATCH RFC] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt

[Hannes Frederic Sowa]

On Sat, Nov 02, 2013 at 05:58:44PM +0100, Florent Fourcot wrote:
> It is already possible to set/put/renew a label
> with IPV6_FLOWLABEL_MGR and setsockopt. This patch
> add the possibility to get information about this
> label (current value, time before expiration, etc).
> 
> It helps application to take decision for a renew
> or a release of the label.
> 
> Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
> ---
>  include/net/ipv6.h       |  1 +
>  net/ipv6/ip6_flowlabel.c | 23 +++++++++++++++++++++++
>  net/ipv6/ipv6_sockglue.c | 23 +++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index dd96638..2a5f668 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -250,6 +250,7 @@ struct ipv6_txoptions *fl6_merge_options(struct ipv6_txoptions *opt_space,
>  					 struct ipv6_txoptions *fopt);
>  void fl6_free_socklist(struct sock *sk);
>  int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen);
> +int ipv6_flowlabel_opt_get(struct sock *sk, struct in6_flowlabel_req *freq);
>  int ip6_flowlabel_init(void);
>  void ip6_flowlabel_cleanup(void);
>  
> diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> index 819578e..34f58b7 100644
> --- a/net/ipv6/ip6_flowlabel.c
> +++ b/net/ipv6/ip6_flowlabel.c
> @@ -475,6 +475,29 @@ static inline void fl_link(struct ipv6_pinfo *np, struct ipv6_fl_socklist *sfl,
>  	spin_unlock_bh(&ip6_sk_fl_lock);
>  }
>  
> +int ipv6_flowlabel_opt_get(struct sock *sk, struct in6_flowlabel_req *freq)
> +{
> +	struct ipv6_pinfo *np = inet6_sk(sk);
> +	struct ipv6_fl_socklist *sfl;
> +
> +	rcu_read_lock_bh();
> +	for_each_sk_fl_rcu(np, sfl) {
> +		if (sfl->fl->label == (np->flow_label & IPV6_FLOWLABEL_MASK)) {

We should take ip6_fl_lock here, otherwise expires extraction races with
the garbage collector (which can update it). There seem to be some other
unsafe places, e.g. fl6_renew.

> +			freq->flr_label = sfl->fl->label;
> +			freq->flr_dst = sfl->fl->dst;
> +			freq->flr_share = sfl->fl->share;
> +			freq->flr_expires = (sfl->fl->expires - jiffies) / HZ;
> +			freq->flr_linger = sfl->fl->linger / HZ;
> +
> +			rcu_read_unlock_bh();
> +			return 0;
> +		}
> +	}
> +	rcu_read_unlock_bh();
> +
> +	return 0;

Maybe return -EINVAL for not found?

> +}
> +
>  int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen)
>  {
>  	int uninitialized_var(err);
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 4919a8e..9ca96c2 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -1212,6 +1212,29 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
>  		val = np->sndflow;
>  		break;
>  
> +	case IPV6_FLOWLABEL_MGR:
> +	{
> +		struct in6_flowlabel_req freq;
> +

Can you think about other actions one might do with this getsockopt? Maybe
we should copy_from_user freq and check if action specifies a GET
request. So a further extension to this API won't break users which did
pass uninitialized memory to this getsockopt.

> +		if (len < sizeof(freq))
> +			return -EINVAL;
> +
> +		len = sizeof(freq);
> +		memset(&freq, 0, sizeof(freq));
> +
> +		val = ipv6_flowlabel_opt_get(sk, &freq);
> +		if (val < 0)
> +			return val;
> +
> +		if (put_user(len, optlen))
> +			return -EFAULT;
> +		if (copy_to_user(optval, &freq, len))
> +			return -EFAULT;
> +
> +		return 0;
> +		break;

I guess return 0 is enough here. The break is superfluous.

Greetings,

  Hannes

Re: [PATCH RFC] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt

[Florent Fourcot]

> We should take ip6_fl_lock here, otherwise expires extraction races with
> the garbage collector (which can update it). There seem to be some other
> unsafe places, e.g. fl6_renew.
> 

I will fix it, and for fl6_renew too.

>> +			freq->flr_label = sfl->fl->label;
>> +			freq->flr_dst = sfl->fl->dst;
>> +			freq->flr_share = sfl->fl->share;
>> +			freq->flr_expires = (sfl->fl->expires - jiffies) / HZ;
>> +			freq->flr_linger = sfl->fl->linger / HZ;
>> +
>> +			rcu_read_unlock_bh();
>> +			return 0;
>> +		}
>> +	}
>> +	rcu_read_unlock_bh();
>> +
>> +	return 0;
> 
> Maybe return -EINVAL for not found?
> 

I don't like -EINVAL for this case, since the user can not know if there
are no label or if the request has bad parameters. Could -ENOMSG be OK?


>> +	case IPV6_FLOWLABEL_MGR:
>> +	{
>> +		struct in6_flowlabel_req freq;
>> +
> 
> Can you think about other actions one might do with this getsockopt? Maybe
> we should copy_from_user freq and check if action specifies a GET
> request. So a further extension to this API won't break users which did
> pass uninitialized memory to this getsockopt.
> 

Very good idea.

Regards,

-- 
Florent.

Re: [PATCH RFC] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt

[Hannes Frederic Sowa]

On Mon, Nov 04, 2013 at 06:09:01PM +0100, Florent Fourcot wrote:
> 
> > We should take ip6_fl_lock here, otherwise expires extraction races with
> > the garbage collector (which can update it). There seem to be some other
> > unsafe places, e.g. fl6_renew.
> > 
> 
> I will fix it, and for fl6_renew too.

Thanks!

> >> +			freq->flr_label = sfl->fl->label;
> >> +			freq->flr_dst = sfl->fl->dst;
> >> +			freq->flr_share = sfl->fl->share;
> >> +			freq->flr_expires = (sfl->fl->expires - jiffies) / HZ;
> >> +			freq->flr_linger = sfl->fl->linger / HZ;
> >> +
> >> +			rcu_read_unlock_bh();
> >> +			return 0;
> >> +		}
> >> +	}
> >> +	rcu_read_unlock_bh();
> >> +
> >> +	return 0;
> > 
> > Maybe return -EINVAL for not found?
> > 
> 
> I don't like -EINVAL for this case, since the user can not know if there
> are no label or if the request has bad parameters. Could -ENOMSG be OK?

-ENOMSG or we already return -ENOENT from the same setsockopt. I would go with
ENOENT.

Greetings,

  Hannes