[PATCH v2 net-next 0/5] l2tp: set l2specific_len based on l2specific_type

[PATCH v2 net-next 0/5] l2tp: set l2specific_len based on l2specific_type

[Lorenzo Bianconi]

Do not rely on l2specific_len value provided by userspace but set sublayer
length according to l2specific_type.
Fix a harmless issue in the switch default case in
l2tp_nl_cmd_session_create().

Changes since v1:
- remove l2specific_len parameter
- add sanity check on l2specific_type provided by userspace

Lorenzo Bianconi (5):
  l2tp: fix switch default error handling in
    l2tp_nl_cmd_session_create()
  l2tp: double-check l2specific_type provided by userspace
  l2tp: remove l2specific_len dependency in l2tp_core
  l2tp: remove l2specific_len configurable parameter
  l2tp: mark L2TP_ATTR_L2SPEC_LEN as not used

 include/uapi/linux/l2tp.h |  2 +-
 net/l2tp/l2tp_core.c      | 35 ++++++++++++++++-------------------
 net/l2tp/l2tp_core.h      | 13 +++++++++++--
 net/l2tp/l2tp_debugfs.c   |  2 +-
 net/l2tp/l2tp_netlink.c   | 17 ++++++++++-------
 5 files changed, 39 insertions(+), 30 deletions(-)

-- 
2.13.6

[PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[Lorenzo Bianconi]

Although this issue is harmless since that code path is protected by the
check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index e1ca29f79821..48b5bf30ec50 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 	case L2TP_PWTYPE_IP:
 	default:
 		ret = -EPROTONOSUPPORT;
-		break;
+		goto out_tunnel;
 	}
 
 	ret = l2tp_nl_cmd_ops[cfg.pw_type]->session_create(net, tunnel,
-- 
2.13.6

[PATCH v2 net-next 2/5] l2tp: double-check l2specific_type provided by userspace

[Lorenzo Bianconi]

Add sanity check on l2specific_type provided by userspace in
l2tp_nl_cmd_session_create() since just L2TP_L2SPECTYPE_DEFAULT and
L2TP_L2SPECTYPE_NONE are currently supported.
Moreover do not always initialize l2specific_type if userspace requests
a given l2-specific sublayer type

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_netlink.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 48b5bf30ec50..711cf208f23a 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -550,9 +550,16 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 		if (info->attrs[L2TP_ATTR_DATA_SEQ])
 			cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
 
-		cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
-		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
+		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
 			cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
+			if (cfg.l2specific_type != L2TP_L2SPECTYPE_DEFAULT &&
+			    cfg.l2specific_type != L2TP_L2SPECTYPE_NONE) {
+				ret = -EINVAL;
+				goto out_tunnel;
+			}
+		} else {
+			cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
+		}
 
 		cfg.l2specific_len = 4;
 		if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
-- 
2.13.6

[PATCH v2 net-next 3/5] l2tp: remove l2specific_len dependency in l2tp_core

[Lorenzo Bianconi]

Remove l2specific_len dependency while building l2tpv3 header or
parsing the received frame since default L2-Specific Sublayer is
always four bytes long and we don't need to rely on a user supplied
value.
Moreover in l2tp netlink code there are no sanity checks to
enforce the relation between l2specific_len and l2specific_type,
so sending a malformed netlink message is possible to set
l2specific_type to L2TP_L2SPECTYPE_DEFAULT (or even
L2TP_L2SPECTYPE_NONE) and set l2specific_len to a value greater than
4 leaking memory on the wire and sending corrupted frames.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_core.c | 34 ++++++++++++++++------------------
 net/l2tp/l2tp_core.h | 11 +++++++++++
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 62285fc6eb59..88efb8b845ca 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -730,11 +730,9 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 				 "%s: recv data ns=%u, session nr=%u\n",
 				 session->name, ns, session->nr);
 		}
+		ptr += 4;
 	}
 
-	/* Advance past L2-specific header, if present */
-	ptr += session->l2specific_len;
-
 	if (L2TP_SKB_CB(skb)->has_seq) {
 		/* Received a packet with sequence numbers. If we're the LNS,
 		 * check if we sre sending sequence numbers and if not,
@@ -1048,21 +1046,20 @@ static int l2tp_build_l2tpv3_header(struct l2tp_session *session, void *buf)
 		memcpy(bufp, &session->cookie[0], session->cookie_len);
 		bufp += session->cookie_len;
 	}
-	if (session->l2specific_len) {
-		if (session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT) {
-			u32 l2h = 0;
-			if (session->send_seq) {
-				l2h = 0x40000000 | session->ns;
-				session->ns++;
-				session->ns &= 0xffffff;
-				l2tp_dbg(session, L2TP_MSG_SEQ,
-					 "%s: updated ns to %u\n",
-					 session->name, session->ns);
-			}
+	if (session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT) {
+		u32 l2h = 0;
 
-			*((__be32 *) bufp) = htonl(l2h);
+		if (session->send_seq) {
+			l2h = 0x40000000 | session->ns;
+			session->ns++;
+			session->ns &= 0xffffff;
+			l2tp_dbg(session, L2TP_MSG_SEQ,
+				 "%s: updated ns to %u\n",
+				 session->name, session->ns);
 		}
-		bufp += session->l2specific_len;
+
+		*((__be32 *)bufp) = htonl(l2h);
+		bufp += 4;
 	}
 
 	return bufp - optr;
@@ -1719,7 +1716,7 @@ int l2tp_session_delete(struct l2tp_session *session)
 EXPORT_SYMBOL_GPL(l2tp_session_delete);
 
 /* We come here whenever a session's send_seq, cookie_len or
- * l2specific_len parameters are set.
+ * l2specific_type parameters are set.
  */
 void l2tp_session_set_header_len(struct l2tp_session *session, int version)
 {
@@ -1728,7 +1725,8 @@ void l2tp_session_set_header_len(struct l2tp_session *session, int version)
 		if (session->send_seq)
 			session->hdr_len += 4;
 	} else {
-		session->hdr_len = 4 + session->cookie_len + session->l2specific_len;
+		session->hdr_len = 4 + session->cookie_len;
+		session->hdr_len += l2tp_get_l2specific_len(session);
 		if (session->tunnel->encap == L2TP_ENCAPTYPE_UDP)
 			session->hdr_len += 4;
 	}
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index c2e9bbd79b35..06128a159a3c 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session)
 		l2tp_session_free(session);
 }
 
+static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
+{
+	switch (session->l2specific_type) {
+	case L2TP_L2SPECTYPE_NONE:
+		return 0;
+	case L2TP_L2SPECTYPE_DEFAULT:
+	default:
+		return 4;
+	}
+}
+
 #define l2tp_printk(ptr, type, func, fmt, ...)				\
 do {									\
 	if (((ptr)->debug) & (type))					\
-- 
2.13.6

[PATCH v2 net-next 4/5] l2tp: remove l2specific_len configurable parameter

[Lorenzo Bianconi]

Remove l2specific_len configuration parameter since now L2-Specific
Sublayer length is computed according to l2specific_type provided by
userspace.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_core.c    | 1 -
 net/l2tp/l2tp_core.h    | 2 --
 net/l2tp/l2tp_debugfs.c | 2 +-
 net/l2tp/l2tp_netlink.c | 4 ----
 4 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 88efb8b845ca..194a7483bb93 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1777,7 +1777,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 			session->lns_mode = cfg->lns_mode;
 			session->reorder_timeout = cfg->reorder_timeout;
 			session->l2specific_type = cfg->l2specific_type;
-			session->l2specific_len = cfg->l2specific_len;
 			session->cookie_len = cfg->cookie_len;
 			memcpy(&session->cookie[0], &cfg->cookie[0], cfg->cookie_len);
 			session->peer_cookie_len = cfg->peer_cookie_len;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 06128a159a3c..f5768fa1d98f 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -59,7 +59,6 @@ struct l2tp_session_cfg {
 	int			debug;		/* bitmask of debug message
 						 * categories */
 	u16			vlan_id;	/* VLAN pseudowire only */
-	u16			l2specific_len;	/* Layer 2 specific length */
 	u16			l2specific_type; /* Layer 2 specific type */
 	u8			cookie[8];	/* optional cookie */
 	int			cookie_len;	/* 0, 4 or 8 bytes */
@@ -85,7 +84,6 @@ struct l2tp_session {
 	int			cookie_len;
 	u8			peer_cookie[8];
 	int			peer_cookie_len;
-	u16			l2specific_len;
 	u16			l2specific_type;
 	u16			hdr_len;
 	u32			nr;		/* session NR state (receive) */
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 2c30587d1a14..72e713da4733 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -181,7 +181,7 @@ static void l2tp_dfs_seq_session_show(struct seq_file *m, void *v)
 		   session->debug,
 		   jiffies_to_msecs(session->reorder_timeout));
 	seq_printf(m, "   offset 0 l2specific %hu/%hu\n",
-		   session->l2specific_type, session->l2specific_len);
+		   session->l2specific_type, l2tp_get_l2specific_len(session));
 	if (session->cookie_len) {
 		seq_printf(m, "   cookie %02x%02x%02x%02x",
 			   session->cookie[0], session->cookie[1],
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 711cf208f23a..f36f0b5f950f 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -561,10 +561,6 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 			cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
 		}
 
-		cfg.l2specific_len = 4;
-		if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
-			cfg.l2specific_len = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_LEN]);
-
 		if (info->attrs[L2TP_ATTR_COOKIE]) {
 			u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
 			if (len > 8) {
-- 
2.13.6

[PATCH v2 net-next 5/5] l2tp: mark L2TP_ATTR_L2SPEC_LEN as not used

[Lorenzo Bianconi]

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 include/uapi/linux/l2tp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 71e62795104d..7d570c7bd117 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -97,7 +97,7 @@ enum {
 	L2TP_ATTR_OFFSET,		/* u16 (not used) */
 	L2TP_ATTR_DATA_SEQ,		/* u16 */
 	L2TP_ATTR_L2SPEC_TYPE,		/* u8, enum l2tp_l2spec_type */
-	L2TP_ATTR_L2SPEC_LEN,		/* u8, enum l2tp_l2spec_type */
+	L2TP_ATTR_L2SPEC_LEN,		/* u8 (not used) */
 	L2TP_ATTR_PROTO_VERSION,	/* u8 */
 	L2TP_ATTR_IFNAME,		/* string */
 	L2TP_ATTR_CONN_ID,		/* u32 */
-- 
2.13.6

Re: [PATCH v2 net-next 2/5] l2tp: double-check l2specific_type provided by userspace

[Guillaume Nault]

On Sun, Jan 14, 2018 at 03:50:55PM +0100, Lorenzo Bianconi wrote:
> Add sanity check on l2specific_type provided by userspace in
> l2tp_nl_cmd_session_create() since just L2TP_L2SPECTYPE_DEFAULT and
> L2TP_L2SPECTYPE_NONE are currently supported.
> Moreover do not always initialize l2specific_type if userspace requests
> a given l2-specific sublayer type
> 
I don't understand your last sentence. l2specific_type is always
initialised in your patch (or session creation is aborted).

> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  net/l2tp/l2tp_netlink.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> index 48b5bf30ec50..711cf208f23a 100644
> --- a/net/l2tp/l2tp_netlink.c
> +++ b/net/l2tp/l2tp_netlink.c
> @@ -550,9 +550,16 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>  		if (info->attrs[L2TP_ATTR_DATA_SEQ])
>  			cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
>  
> -		cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
> -		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
> +		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
>  			cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
> +			if (cfg.l2specific_type != L2TP_L2SPECTYPE_DEFAULT &&
> +			    cfg.l2specific_type != L2TP_L2SPECTYPE_NONE) {
> +				ret = -EINVAL;
> +				goto out_tunnel;
> +			}
> +		} else {
> +			cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
> +		}
>  
>  		cfg.l2specific_len = 4;
>  		if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
> -- 
> 2.13.6
> 

Re: [PATCH v2 net-next 2/5] l2tp: double-check l2specific_type provided by userspace

[Lorenzo Bianconi]

> On Sun, Jan 14, 2018 at 03:50:55PM +0100, Lorenzo Bianconi wrote:
>> Add sanity check on l2specific_type provided by userspace in
>> l2tp_nl_cmd_session_create() since just L2TP_L2SPECTYPE_DEFAULT and
>> L2TP_L2SPECTYPE_NONE are currently supported.
>> Moreover do not always initialize l2specific_type if userspace requests
>> a given l2-specific sublayer type
>>
> I don't understand your last sentence. l2specific_type is always
> initialised in your patch (or session creation is aborted).
>

I mean to explicitly set l2specific_type to L2TP_L2SPECTYPE_DEFAULT
only if the userspace does not provide a value for it (I moved the
'default' initialization in the 'else' case)

>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> ---
>>  net/l2tp/l2tp_netlink.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
>> index 48b5bf30ec50..711cf208f23a 100644
>> --- a/net/l2tp/l2tp_netlink.c
>> +++ b/net/l2tp/l2tp_netlink.c
>> @@ -550,9 +550,16 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>>               if (info->attrs[L2TP_ATTR_DATA_SEQ])
>>                       cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
>>
>> -             cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
>> -             if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
>> +             if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
>>                       cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
>> +                     if (cfg.l2specific_type != L2TP_L2SPECTYPE_DEFAULT &&
>> +                         cfg.l2specific_type != L2TP_L2SPECTYPE_NONE) {
>> +                             ret = -EINVAL;
>> +                             goto out_tunnel;
>> +                     }
>> +             } else {
>> +                     cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
>> +             }
>>
>>               cfg.l2specific_len = 4;
>>               if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
>> --
>> 2.13.6
>>

Re: [PATCH v2 net-next 3/5] l2tp: remove l2specific_len dependency in l2tp_core

[Guillaume Nault]

On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote:
> --- a/net/l2tp/l2tp_core.h
> +++ b/net/l2tp/l2tp_core.h
> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session)
>  		l2tp_session_free(session);
>  }
>  
> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
> +{
> +	switch (session->l2specific_type) {
> +	case L2TP_L2SPECTYPE_NONE:
> +		return 0;
> +	case L2TP_L2SPECTYPE_DEFAULT:
> +	default:
> +		return 4;
> +	}
> +}
> 
The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and
treats any other value as L2SPECTYPE_NONE. Therefore, we should keep
this logic here and return 0 for unknown types.

Re: [PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[Guillaume Nault]

On Sun, Jan 14, 2018 at 03:50:54PM +0100, Lorenzo Bianconi wrote:
> Although this issue is harmless since that code path is protected by the
> check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
> handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  net/l2tp/l2tp_netlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> index e1ca29f79821..48b5bf30ec50 100644
> --- a/net/l2tp/l2tp_netlink.c
> +++ b/net/l2tp/l2tp_netlink.c
> @@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>  	case L2TP_PWTYPE_IP:
>  	default:
>  		ret = -EPROTONOSUPPORT;
> -		break;
> +		goto out_tunnel;
>  	}
>
Not sure if this change is really worthwhile. As you noted, this is
unreachable code. And this switch should better be removed entirely:
it doesn't do anything for supported pseudo-wires.

And if PWTYPE_ETH_VLAN were to be implemented, it should perform its
configuration consistency checking in its own PW specific code, not in
the genl handler.

Anyway, removing this switch isn't the purpose of this series, so I
think you can drop this patch.

Re: [PATCH v2 net-next 3/5] l2tp: remove l2specific_len dependency in l2tp_core

[Lorenzo Bianconi]

> On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote:
>> --- a/net/l2tp/l2tp_core.h
>> +++ b/net/l2tp/l2tp_core.h
>> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session)
>>               l2tp_session_free(session);
>>  }
>>
>> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
>> +{
>> +     switch (session->l2specific_type) {
>> +     case L2TP_L2SPECTYPE_NONE:
>> +             return 0;
>> +     case L2TP_L2SPECTYPE_DEFAULT:
>> +     default:
>> +             return 4;
>> +     }
>> +}
>>
> The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and
> treats any other value as L2SPECTYPE_NONE. Therefore, we should keep
> this logic here and return 0 for unknown types.

The data path only compares l2specific_type to L2SPECTYPE_DEFAULT
since in the other supported case (L2SPECTYPE_NONE) there is no action
required. Moreover L2SPECTYPE_DEFAULT is default configured value if
the user does not provide any value for l2specific_type so there are
no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better
choice for default value

Re: [PATCH v2 net-next 2/5] l2tp: double-check l2specific_type provided by userspace

[Guillaume Nault]

On Mon, Jan 15, 2018 at 07:18:17PM +0100, Lorenzo Bianconi wrote:
> > On Sun, Jan 14, 2018 at 03:50:55PM +0100, Lorenzo Bianconi wrote:
> >> Add sanity check on l2specific_type provided by userspace in
> >> l2tp_nl_cmd_session_create() since just L2TP_L2SPECTYPE_DEFAULT and
> >> L2TP_L2SPECTYPE_NONE are currently supported.
> >> Moreover do not always initialize l2specific_type if userspace requests
> >> a given l2-specific sublayer type
> >>
> > I don't understand your last sentence. l2specific_type is always
> > initialised in your patch (or session creation is aborted).
> >
> 
> I mean to explicitly set l2specific_type to L2TP_L2SPECTYPE_DEFAULT
> only if the userspace does not provide a value for it (I moved the
> 'default' initialization in the 'else' case)
> 
Ok, I thought you were talking about a functional change.

Re: [PATCH v2 net-next 3/5] l2tp: remove l2specific_len dependency in l2tp_core

[Guillaume Nault]

On Mon, Jan 15, 2018 at 07:43:18PM +0100, Lorenzo Bianconi wrote:
> > On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote:
> >> --- a/net/l2tp/l2tp_core.h
> >> +++ b/net/l2tp/l2tp_core.h
> >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session)
> >>               l2tp_session_free(session);
> >>  }
> >>
> >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
> >> +{
> >> +     switch (session->l2specific_type) {
> >> +     case L2TP_L2SPECTYPE_NONE:
> >> +             return 0;
> >> +     case L2TP_L2SPECTYPE_DEFAULT:
> >> +     default:
> >> +             return 4;
> >> +     }
> >> +}
> >>
> > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and
> > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep
> > this logic here and return 0 for unknown types.
> 
> The data path only compares l2specific_type to L2SPECTYPE_DEFAULT
> since in the other supported case (L2SPECTYPE_NONE) there is no action
> required. Moreover L2SPECTYPE_DEFAULT is default configured value if
> the user does not provide any value for l2specific_type so there are
> no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better
> choice for default value
> 
Yes, but what I meant is that the data patch treats unknow values as
L2SPECTYPE_NONE, while l2tp_get_l2specific_len() now treats them as
L2TP_L2SPECTYPE_DEFAULT. I'd just prefer to avoid that inconsistency;
it makes it easier to reason about the code.

But if you really prefer L2SPECTYPE_DEFAULT, then fine. Unless someone
messes with new l2spec types, we should never reach this case.

Re: [PATCH v2 net-next 3/5] l2tp: remove l2specific_len dependency in l2tp_core

[Lorenzo Bianconi]

> On Mon, Jan 15, 2018 at 07:43:18PM +0100, Lorenzo Bianconi wrote:
>> > On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote:
>> >> --- a/net/l2tp/l2tp_core.h
>> >> +++ b/net/l2tp/l2tp_core.h
>> >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session)
>> >>               l2tp_session_free(session);
>> >>  }
>> >>
>> >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
>> >> +{
>> >> +     switch (session->l2specific_type) {
>> >> +     case L2TP_L2SPECTYPE_NONE:
>> >> +             return 0;
>> >> +     case L2TP_L2SPECTYPE_DEFAULT:
>> >> +     default:
>> >> +             return 4;
>> >> +     }
>> >> +}
>> >>
>> > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and
>> > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep
>> > this logic here and return 0 for unknown types.
>>
>> The data path only compares l2specific_type to L2SPECTYPE_DEFAULT
>> since in the other supported case (L2SPECTYPE_NONE) there is no action
>> required. Moreover L2SPECTYPE_DEFAULT is default configured value if
>> the user does not provide any value for l2specific_type so there are
>> no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better
>> choice for default value
>>
> Yes, but what I meant is that the data patch treats unknow values as
> L2SPECTYPE_NONE, while l2tp_get_l2specific_len() now treats them as
> L2TP_L2SPECTYPE_DEFAULT. I'd just prefer to avoid that inconsistency;
> it makes it easier to reason about the code.
>
> But if you really prefer L2SPECTYPE_DEFAULT, then fine. Unless someone
> messes with new l2spec types, we should never reach this case.

Yes, there are now way to hit just default case so there are no
difference to use L2SPECTYPE_DEFAULT or L2SPECTYPE_NONE as default
actually.
Moreover from now on there are no 'unknow' values for l2specific_type.
Anyway if you think that change is important I can respin a v3, no
issue from my side.

Regards,
Lorenzo

Re: [PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[Lorenzo Bianconi]

> On Sun, Jan 14, 2018 at 03:50:54PM +0100, Lorenzo Bianconi wrote:
>> Although this issue is harmless since that code path is protected by the
>> check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
>> handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> ---
>>  net/l2tp/l2tp_netlink.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
>> index e1ca29f79821..48b5bf30ec50 100644
>> --- a/net/l2tp/l2tp_netlink.c
>> +++ b/net/l2tp/l2tp_netlink.c
>> @@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>>       case L2TP_PWTYPE_IP:
>>       default:
>>               ret = -EPROTONOSUPPORT;
>> -             break;
>> +             goto out_tunnel;
>>       }
>>
> Not sure if this change is really worthwhile. As you noted, this is
> unreachable code. And this switch should better be removed entirely:
> it doesn't do anything for supported pseudo-wires.
>
> And if PWTYPE_ETH_VLAN were to be implemented, it should perform its
> configuration consistency checking in its own PW specific code, not in
> the genl handler.
>

Personally I would prefer to not remove some code that could be useful
for a future implementation, but just fix it if it presents issues to
address.
Anyway we can simply drop this patch from the series and I can send a
new one to remove the switch entirely.

@James what do you think?

Regards,
Lorenzo

> Anyway, removing this switch isn't the purpose of this series, so I
> think you can drop this patch.

Re: [PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[James Chapman]

On 15 January 2018 at 21:18, Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>> On Sun, Jan 14, 2018 at 03:50:54PM +0100, Lorenzo Bianconi wrote:
>>> Although this issue is harmless since that code path is protected by the
>>> check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
>>> handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()
>>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>>> ---
>>>  net/l2tp/l2tp_netlink.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
>>> index e1ca29f79821..48b5bf30ec50 100644
>>> --- a/net/l2tp/l2tp_netlink.c
>>> +++ b/net/l2tp/l2tp_netlink.c
>>> @@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>>>       case L2TP_PWTYPE_IP:
>>>       default:
>>>               ret = -EPROTONOSUPPORT;
>>> -             break;
>>> +             goto out_tunnel;
>>>       }
>>>
>> Not sure if this change is really worthwhile. As you noted, this is
>> unreachable code. And this switch should better be removed entirely:
>> it doesn't do anything for supported pseudo-wires.
>>
>> And if PWTYPE_ETH_VLAN were to be implemented, it should perform its
>> configuration consistency checking in its own PW specific code, not in
>> the genl handler.
>>
>
> Personally I would prefer to not remove some code that could be useful
> for a future implementation, but just fix it if it presents issues to
> address.
> Anyway we can simply drop this patch from the series and I can send a
> new one to remove the switch entirely.
>
> @James what do you think?

Keep the patch series focused. If you read the patch series as a set,
this patch stands out as not fitting the purpose of the series. I
agree with Guillaume.

>
> Regards,
> Lorenzo
>
>> Anyway, removing this switch isn't the purpose of this series, so I
>> think you can drop this patch.

I agree.

Re: [PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[Lorenzo Bianconi]

> On 15 January 2018 at 21:18, Lorenzo Bianconi
> <lorenzo.bianconi@redhat.com> wrote:
>>> On Sun, Jan 14, 2018 at 03:50:54PM +0100, Lorenzo Bianconi wrote:
>>>> Although this issue is harmless since that code path is protected by the
>>>> check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
>>>> handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()
>>>>
>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>>>> ---
>>>>  net/l2tp/l2tp_netlink.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
>>>> index e1ca29f79821..48b5bf30ec50 100644
>>>> --- a/net/l2tp/l2tp_netlink.c
>>>> +++ b/net/l2tp/l2tp_netlink.c
>>>> @@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>>>>       case L2TP_PWTYPE_IP:
>>>>       default:
>>>>               ret = -EPROTONOSUPPORT;
>>>> -             break;
>>>> +             goto out_tunnel;
>>>>       }
>>>>
>>> Not sure if this change is really worthwhile. As you noted, this is
>>> unreachable code. And this switch should better be removed entirely:
>>> it doesn't do anything for supported pseudo-wires.
>>>
>>> And if PWTYPE_ETH_VLAN were to be implemented, it should perform its
>>> configuration consistency checking in its own PW specific code, not in
>>> the genl handler.
>>>
>>
>> Personally I would prefer to not remove some code that could be useful
>> for a future implementation, but just fix it if it presents issues to
>> address.
>> Anyway we can simply drop this patch from the series and I can send a
>> new one to remove the switch entirely.
>>
>> @James what do you think?
>
> Keep the patch series focused. If you read the patch series as a set,
> this patch stands out as not fitting the purpose of the series. I
> agree with Guillaume.
>

Ok, fine. What about the fix? Do you prefer to remove the switch or just fix it?

>>
>> Regards,
>> Lorenzo
>>
>>> Anyway, removing this switch isn't the purpose of this series, so I
>>> think you can drop this patch.
>
> I agree.

Re: [PATCH v2 net-next 3/5] l2tp: remove l2specific_len dependency in l2tp_core

[Guillaume Nault]

On Mon, Jan 15, 2018 at 10:11:04PM +0100, Lorenzo Bianconi wrote:
> > On Mon, Jan 15, 2018 at 07:43:18PM +0100, Lorenzo Bianconi wrote:
> >> > On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote:
> >> >> --- a/net/l2tp/l2tp_core.h
> >> >> +++ b/net/l2tp/l2tp_core.h
> >> >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session)
> >> >>               l2tp_session_free(session);
> >> >>  }
> >> >>
> >> >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
> >> >> +{
> >> >> +     switch (session->l2specific_type) {
> >> >> +     case L2TP_L2SPECTYPE_NONE:
> >> >> +             return 0;
> >> >> +     case L2TP_L2SPECTYPE_DEFAULT:
> >> >> +     default:
> >> >> +             return 4;
> >> >> +     }
> >> >> +}
> >> >>
> >> > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and
> >> > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep
> >> > this logic here and return 0 for unknown types.
> >>
> >> The data path only compares l2specific_type to L2SPECTYPE_DEFAULT
> >> since in the other supported case (L2SPECTYPE_NONE) there is no action
> >> required. Moreover L2SPECTYPE_DEFAULT is default configured value if
> >> the user does not provide any value for l2specific_type so there are
> >> no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better
> >> choice for default value
> >>
> > Yes, but what I meant is that the data patch treats unknow values as
> > L2SPECTYPE_NONE, while l2tp_get_l2specific_len() now treats them as
> > L2TP_L2SPECTYPE_DEFAULT. I'd just prefer to avoid that inconsistency;
> > it makes it easier to reason about the code.
> >
> > But if you really prefer L2SPECTYPE_DEFAULT, then fine. Unless someone
> > messes with new l2spec types, we should never reach this case.
> 
> Yes, there are now way to hit just default case so there are no
> difference to use L2SPECTYPE_DEFAULT or L2SPECTYPE_NONE as default
> actually.
> Moreover from now on there are no 'unknow' values for l2specific_type.
> 
Everything that isn't L2SPECTYPE_NONE or L2SPECTYPE_DEFAULT is unknown.
The datapath reads them as L2SPECTYPE_NONE and
l2tp_get_l2specific_len() as L2SPECTYPE_DEFAULT. That's inconsistent
and one has to read the rest of the code to figure out that, in fact,
that's not an issue because such values actually can't be configured.
Using "session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT ? 4 : 0"
would have been enough IMO.

> Anyway if you think that change is important I can respin a v3, no
> issue from my side.
> 
It's certainly not worth a v3 on its own. I just give you my
preference, but the patchset is yours and I can live with either
version.

Re: [PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[Guillaume Nault]

On Mon, Jan 15, 2018 at 10:18:53PM +0100, Lorenzo Bianconi wrote:
> > On Sun, Jan 14, 2018 at 03:50:54PM +0100, Lorenzo Bianconi wrote:
> >> Although this issue is harmless since that code path is protected by the
> >> check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
> >> handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()
> >>
> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >> ---
> >>  net/l2tp/l2tp_netlink.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> >> index e1ca29f79821..48b5bf30ec50 100644
> >> --- a/net/l2tp/l2tp_netlink.c
> >> +++ b/net/l2tp/l2tp_netlink.c
> >> @@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
> >>       case L2TP_PWTYPE_IP:
> >>       default:
> >>               ret = -EPROTONOSUPPORT;
> >> -             break;
> >> +             goto out_tunnel;
> >>       }
> >>
> > Not sure if this change is really worthwhile. As you noted, this is
> > unreachable code. And this switch should better be removed entirely:
> > it doesn't do anything for supported pseudo-wires.
> >
> > And if PWTYPE_ETH_VLAN were to be implemented, it should perform its
> > configuration consistency checking in its own PW specific code, not in
> > the genl handler.
> >
> 
> Personally I would prefer to not remove some code that could be useful
> for a future implementation, but just fix it if it presents issues to
> address.
> 
I don't think this code can be useful in the future, quite the other
way around. l2tp_nl_cmd_session_create() has to make sure that it can
safely call ->session_create(), nothing more. Removing this code would
force a new PW implementation to do its configuration checking at the
right place.

BTW, writing code speculatively is generally not a good idea. Such code
can't be tested, and the future tends to be quite different from what
was expeceted anyway.

Re: [PATCH v2 net-next 0/5] l2tp: set l2specific_len based on l2specific_type

[Guillaume Nault]

On Sun, Jan 14, 2018 at 03:50:53PM +0100, Lorenzo Bianconi wrote:
> Do not rely on l2specific_len value provided by userspace but set sublayer
> length according to l2specific_type.
> Fix a harmless issue in the switch default case in
> l2tp_nl_cmd_session_create().
> 
> Changes since v1:
> - remove l2specific_len parameter
> - add sanity check on l2specific_type provided by userspace
> 
Thanks for working on this Lorenzo.

Whatever the discussions on patches 1 and 3 lead to:

Reviewed-by: Guillaume Nault <g.nault@alphalink.fr>
Tested-by: Guillaume Nault <g.nault@alphalink.fr>

Re: [PATCH v2 net-next 0/5] l2tp: set l2specific_len based on l2specific_type

[Lorenzo Bianconi]

> On Sun, Jan 14, 2018 at 03:50:53PM +0100, Lorenzo Bianconi wrote:
>> Do not rely on l2specific_len value provided by userspace but set sublayer
>> length according to l2specific_type.
>> Fix a harmless issue in the switch default case in
>> l2tp_nl_cmd_session_create().
>>
>> Changes since v1:
>> - remove l2specific_len parameter
>> - add sanity check on l2specific_type provided by userspace
>>
> Thanks for working on this Lorenzo.
>
> Whatever the discussions on patches 1 and 3 lead to:
>
> Reviewed-by: Guillaume Nault <g.nault@alphalink.fr>
> Tested-by: Guillaume Nault <g.nault@alphalink.fr>

I will send a v3 later today dropping the first patch (I will send a
single one removing the switch later) and including your suggestion
for default case in  l2tp_get_l2specific_len().
Thanks for reviewing the patch :)

Regards,
Lorenzo

Re: [PATCH v2 net-next 0/5] l2tp: set l2specific_len based on l2specific_type

[Guillaume Nault]

On Tue, Jan 16, 2018 at 12:45:06PM +0100, Lorenzo Bianconi wrote:
> > On Sun, Jan 14, 2018 at 03:50:53PM +0100, Lorenzo Bianconi wrote:
> >> Do not rely on l2specific_len value provided by userspace but set sublayer
> >> length according to l2specific_type.
> >> Fix a harmless issue in the switch default case in
> >> l2tp_nl_cmd_session_create().
> >>
> >> Changes since v1:
> >> - remove l2specific_len parameter
> >> - add sanity check on l2specific_type provided by userspace
> >>
> > Thanks for working on this Lorenzo.
> >
> > Whatever the discussions on patches 1 and 3 lead to:
> >
> > Reviewed-by: Guillaume Nault <g.nault@alphalink.fr>
> > Tested-by: Guillaume Nault <g.nault@alphalink.fr>
> 
> I will send a v3 later today dropping the first patch (I will send a
> single one removing the switch later) and including your suggestion
> for default case in  l2tp_get_l2specific_len().
> Thanks for reviewing the patch :)
> 
Great! Feel free to keep my Reviewed-by: and Tested-by: tags.

Regards,

Guillaume