netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
@ 2020-01-13 12:45 Martin Schiller
  2020-01-13 12:45 ` [PATCH 2/2] wan/hdlc_x25: fix skb handling Martin Schiller
  2020-01-13 13:53 ` [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Jakub Kicinski
  0 siblings, 2 replies; 10+ messages in thread
From: Martin Schiller @ 2020-01-13 12:45 UTC (permalink / raw)
  To: khc, davem; +Cc: linux-x25, netdev, linux-kernel, Martin Schiller

This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2, N2 via
sethdlc (which needs to be patched as well).

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 drivers/net/wan/hdlc_x25.c      | 67 ++++++++++++++++++++++++++++++++-
 include/uapi/linux/hdlc/ioctl.h | 11 +++++-
 include/uapi/linux/if.h         |  1 +
 3 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 5643675ff724..b28051eba736 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -21,8 +21,17 @@
 #include <linux/skbuff.h>
 #include <net/x25device.h>
 
+struct x25_state {
+	x25_hdlc_proto settings;
+};
+
 static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
 
+static inline struct x25_state* state(hdlc_device *hdlc)
+{
+	return (struct x25_state *)hdlc->state;
+}
+
 /* These functions are callbacks called by LAPB layer */
 
 static void x25_connect_disconnect(struct net_device *dev, int reason, int code)
@@ -132,6 +141,8 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
 static int x25_open(struct net_device *dev)
 {
 	int result;
+	hdlc_device *hdlc = dev_to_hdlc(dev);
+	struct lapb_parms_struct params;
 	static const struct lapb_register_struct cb = {
 		.connect_confirmation = x25_connected,
 		.connect_indication = x25_connected,
@@ -144,6 +155,26 @@ static int x25_open(struct net_device *dev)
 	result = lapb_register(dev, &cb);
 	if (result != LAPB_OK)
 		return result;
+
+	result = lapb_getparms(dev, &params);
+	if (result != LAPB_OK)
+		return result;
+
+	if (state(hdlc)->settings.dce)
+		params.mode = params.mode | LAPB_DCE;
+
+	if (state(hdlc)->settings.modulo == 128)
+		params.mode = params.mode | LAPB_EXTENDED;
+
+	params.window = state(hdlc)->settings.window;
+	params.t1 = state(hdlc)->settings.t1;
+	params.t2 = state(hdlc)->settings.t2;
+	params.n2 = state(hdlc)->settings.n2;
+
+	result = lapb_setparms(dev, &params);
+	if (result != LAPB_OK)
+		return result;
+
 	return 0;
 }
 
@@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
 
 static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
 {
+	x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
+	const size_t size = sizeof(x25_hdlc_proto);
+	x25_hdlc_proto new_settings;
 	hdlc_device *hdlc = dev_to_hdlc(dev);
 	int result;
 
@@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
 		if (dev_to_hdlc(dev)->proto != &proto)
 			return -EINVAL;
 		ifr->ifr_settings.type = IF_PROTO_X25;
-		return 0; /* return protocol only, no settable parameters */
+		if (ifr->ifr_settings.size < size) {
+			ifr->ifr_settings.size = size; /* data size wanted */
+			return -ENOBUFS;
+		}
+		if (copy_to_user(x25_s, &state(hdlc)->settings, size))
+			return -EFAULT;
+		return 0;
 
 	case IF_PROTO_X25:
 		if (!capable(CAP_NET_ADMIN))
@@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
 
+		if (copy_from_user(&new_settings, x25_s, size))
+			return -EFAULT;
+
+		if ((new_settings.dce != 0 &&
+		     new_settings.dce != 1) ||
+		    (new_settings.modulo != 8 &&
+		     new_settings.modulo != 128) ||
+		    new_settings.window < 1 ||
+		    (new_settings.modulo == 8 &&
+		     new_settings.window > 7) ||
+		    (new_settings.modulo == 128 &&
+		     new_settings.window > 127) ||
+		    new_settings.t1 < 1 ||
+		    new_settings.t1 > 255 ||
+		    new_settings.t2 < 1 ||
+		    new_settings.t2 > 255 ||
+		    new_settings.n2 < 1 ||
+		    new_settings.n2 > 255)
+			return -EINVAL;
+
 		result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
 		if (result)
 			return result;
 
-		if ((result = attach_hdlc_protocol(dev, &proto, 0)))
+		if ((result = attach_hdlc_protocol(dev, &proto,
+						   sizeof(struct x25_state))))
 			return result;
+
+		memcpy(&state(hdlc)->settings, &new_settings, size);
 		dev->type = ARPHRD_X25;
 		call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
 		netif_dormant_off(dev);
diff --git a/include/uapi/linux/hdlc/ioctl.h b/include/uapi/linux/hdlc/ioctl.h
index 0fe4238e8246..3656ce8b8af0 100644
--- a/include/uapi/linux/hdlc/ioctl.h
+++ b/include/uapi/linux/hdlc/ioctl.h
@@ -3,7 +3,7 @@
 #define __HDLC_IOCTL_H__
 
 
-#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc utility */
+#define GENERIC_HDLC_VERSION 5	/* For synchronization with sethdlc utility */
 
 #define CLOCK_DEFAULT   0	/* Default setting */
 #define CLOCK_EXT	1	/* External TX and RX clock - DTE */
@@ -79,6 +79,15 @@ typedef struct {
     unsigned int timeout;
 } cisco_proto;
 
+typedef struct {
+	unsigned short dce; /* 1 for DCE (network side) operation */
+	unsigned int modulo; /* modulo (8 = basic / 128 = extended) */
+	unsigned int window; /* frame window size */
+	unsigned int t1; /* timeout t1 */
+	unsigned int t2; /* timeout t2 */
+	unsigned int n2; /* frame retry counter */
+} x25_hdlc_proto;
+
 /* PPP doesn't need any info now - supply length = 0 to ioctl */
 
 #endif /* __ASSEMBLY__ */
diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index 4bf33344aab1..be714cd8c826 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -213,6 +213,7 @@ struct if_settings {
 		fr_proto		__user *fr;
 		fr_proto_pvc		__user *fr_pvc;
 		fr_proto_pvc_info	__user *fr_pvc_info;
+		x25_hdlc_proto		__user *x25;
 
 		/* interface settings */
 		sync_serial_settings	__user *sync;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] wan/hdlc_x25: fix skb handling
  2020-01-13 12:45 [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Martin Schiller
@ 2020-01-13 12:45 ` Martin Schiller
  2020-01-13 13:44   ` Jakub Kicinski
  2020-01-13 13:53 ` [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Jakub Kicinski
  1 sibling, 1 reply; 10+ messages in thread
From: Martin Schiller @ 2020-01-13 12:45 UTC (permalink / raw)
  To: khc, davem; +Cc: linux-x25, netdev, linux-kernel, Martin Schiller

 o call skb_reset_network_header() before hdlc->xmit()
 o change skb proto to HDLC (0x0019) before hdlc->xmit()
 o call dev_queue_xmit_nit() before hdlc->xmit()

This changes make it possible to trace (tcpdump) outgoing layer2
(ETH_P_HDLC) packets

 o use a copy of the skb for lapb_data_request() in x25_xmit()

This fixes the problem, that tracing layer3 (ETH_P_X25) packets
results in a malformed first byte of the packets.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 drivers/net/wan/hdlc_x25.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index b28051eba736..434e5263eddf 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -72,6 +72,7 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
 	unsigned char *ptr;
 
 	skb_push(skb, 1);
+	skb_reset_network_header(skb);
 
 	if (skb_cow(skb, 1))
 		return NET_RX_DROP;
@@ -88,6 +89,9 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
 static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
 {
 	hdlc_device *hdlc = dev_to_hdlc(dev);
+	skb_reset_network_header(skb);
+	skb->protocol = hdlc_type_trans(skb, dev);
+	dev_queue_xmit_nit(skb, dev);
 	hdlc->xmit(skb, dev); /* Ignore return value :-( */
 }
 
@@ -95,16 +99,19 @@ static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
 
 static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	struct sk_buff *skbn;
 	int result;
 
 
 	/* X.25 to LAPB */
 	switch (skb->data[0]) {
 	case X25_IFACE_DATA:	/* Data to be transmitted */
-		skb_pull(skb, 1);
-		if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
-			dev_kfree_skb(skb);
-		return NETDEV_TX_OK;
+		skbn = skb_copy(skb, GFP_ATOMIC);
+		skb_pull(skbn, 1);
+		skb_reset_network_header(skbn);
+		if ((result = lapb_data_request(dev, skbn)) != LAPB_OK)
+			dev_kfree_skb(skbn);
+		break;
 
 	case X25_IFACE_CONNECT:
 		if ((result = lapb_connect_request(dev))!= LAPB_OK) {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] wan/hdlc_x25: fix skb handling
  2020-01-13 12:45 ` [PATCH 2/2] wan/hdlc_x25: fix skb handling Martin Schiller
@ 2020-01-13 13:44   ` Jakub Kicinski
  2020-01-14  8:10     ` Martin Schiller
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-01-13 13:44 UTC (permalink / raw)
  To: Martin Schiller; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On Mon, 13 Jan 2020 13:45:51 +0100, Martin Schiller wrote:
>  o call skb_reset_network_header() before hdlc->xmit()
>  o change skb proto to HDLC (0x0019) before hdlc->xmit()
>  o call dev_queue_xmit_nit() before hdlc->xmit()
> 
> This changes make it possible to trace (tcpdump) outgoing layer2
> (ETH_P_HDLC) packets
> 
>  o use a copy of the skb for lapb_data_request() in x25_xmit()

It's not clear to me why

> This fixes the problem, that tracing layer3 (ETH_P_X25) packets
> results in a malformed first byte of the packets.
> 
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> ---
>  drivers/net/wan/hdlc_x25.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
> index b28051eba736..434e5263eddf 100644
> --- a/drivers/net/wan/hdlc_x25.c
> +++ b/drivers/net/wan/hdlc_x25.c
> @@ -72,6 +72,7 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
>  	unsigned char *ptr;
>  
>  	skb_push(skb, 1);
> +	skb_reset_network_header(skb);
>  
>  	if (skb_cow(skb, 1))

This skb_cow() here is for the next handler down to have a 1 byte of
headroom guaranteed? It'd seem more natural to have skb_cow before the
push.. not that it's related to your patch.

>  		return NET_RX_DROP;
> @@ -88,6 +89,9 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
>  static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
>  {
>  	hdlc_device *hdlc = dev_to_hdlc(dev);

Please insert a new line after the variable declaration since you're
touching this one.

> +	skb_reset_network_header(skb);
> +	skb->protocol = hdlc_type_trans(skb, dev);
> +	dev_queue_xmit_nit(skb, dev);
>  	hdlc->xmit(skb, dev); /* Ignore return value :-( */
>  }
>  

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
  2020-01-13 12:45 [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Martin Schiller
  2020-01-13 12:45 ` [PATCH 2/2] wan/hdlc_x25: fix skb handling Martin Schiller
@ 2020-01-13 13:53 ` Jakub Kicinski
  2020-01-14  5:37   ` Martin Schiller
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-01-13 13:53 UTC (permalink / raw)
  To: Martin Schiller; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On Mon, 13 Jan 2020 13:45:50 +0100, Martin Schiller wrote:
> This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2, N2 via
> sethdlc (which needs to be patched as well).
> 
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>

> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
> index 5643675ff724..b28051eba736 100644
> --- a/drivers/net/wan/hdlc_x25.c
> +++ b/drivers/net/wan/hdlc_x25.c
> @@ -21,8 +21,17 @@
>  #include <linux/skbuff.h>
>  #include <net/x25device.h>
>  
> +struct x25_state {
> +	x25_hdlc_proto settings;
> +};
> +
>  static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
>  
> +static inline struct x25_state* state(hdlc_device *hdlc)

Please no more static inlines in source files. Compiler will know what
to do.

> +{
> +	return (struct x25_state *)hdlc->state;
> +}
> +
>  /* These functions are callbacks called by LAPB layer */
>  
>  static void x25_connect_disconnect(struct net_device *dev, int reason, int code)

> @@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
>  
>  static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
>  {
> +	x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
> +	const size_t size = sizeof(x25_hdlc_proto);
> +	x25_hdlc_proto new_settings;
>  	hdlc_device *hdlc = dev_to_hdlc(dev);
>  	int result;
>  
> @@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
>  		if (dev_to_hdlc(dev)->proto != &proto)
>  			return -EINVAL;
>  		ifr->ifr_settings.type = IF_PROTO_X25;
> -		return 0; /* return protocol only, no settable parameters */
> +		if (ifr->ifr_settings.size < size) {
> +			ifr->ifr_settings.size = size; /* data size wanted */
> +			return -ENOBUFS;
> +		}
> +		if (copy_to_user(x25_s, &state(hdlc)->settings, size))
> +			return -EFAULT;
> +		return 0;
>  
>  	case IF_PROTO_X25:
>  		if (!capable(CAP_NET_ADMIN))
> @@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
>  		if (dev->flags & IFF_UP)
>  			return -EBUSY;
>  
> +		if (copy_from_user(&new_settings, x25_s, size))
> +			return -EFAULT;
> +
> +		if ((new_settings.dce != 0 &&
> +		     new_settings.dce != 1) ||
> +		    (new_settings.modulo != 8 &&
> +		     new_settings.modulo != 128) ||
> +		    new_settings.window < 1 ||
> +		    (new_settings.modulo == 8 &&
> +		     new_settings.window > 7) ||
> +		    (new_settings.modulo == 128 &&
> +		     new_settings.window > 127) ||
> +		    new_settings.t1 < 1 ||
> +		    new_settings.t1 > 255 ||
> +		    new_settings.t2 < 1 ||
> +		    new_settings.t2 > 255 ||
> +		    new_settings.n2 < 1 ||
> +		    new_settings.n2 > 255)
> +			return -EINVAL;
> +
>  		result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
>  		if (result)
>  			return result;
>  
> -		if ((result = attach_hdlc_protocol(dev, &proto, 0)))
> +		if ((result = attach_hdlc_protocol(dev, &proto,
> +						   sizeof(struct x25_state))))
>  			return result;
> +
> +		memcpy(&state(hdlc)->settings, &new_settings, size);
>  		dev->type = ARPHRD_X25;
>  		call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
>  		netif_dormant_off(dev);
> diff --git a/include/uapi/linux/hdlc/ioctl.h b/include/uapi/linux/hdlc/ioctl.h
> index 0fe4238e8246..3656ce8b8af0 100644
> --- a/include/uapi/linux/hdlc/ioctl.h
> +++ b/include/uapi/linux/hdlc/ioctl.h
> @@ -3,7 +3,7 @@
>  #define __HDLC_IOCTL_H__
>  
>  
> -#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc utility */
> +#define GENERIC_HDLC_VERSION 5	/* For synchronization with sethdlc utility */

What's the backward compatibility story in this code?

The IOCTL handling at least looks like it may start returning errors
to existing user space which could have expected the parameters to
IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.

>  #define CLOCK_DEFAULT   0	/* Default setting */
>  #define CLOCK_EXT	1	/* External TX and RX clock - DTE */

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
  2020-01-13 13:53 ` [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Jakub Kicinski
@ 2020-01-14  5:37   ` Martin Schiller
  2020-01-14 12:51     ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Martin Schiller @ 2020-01-14  5:37 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On 2020-01-13 14:53, Jakub Kicinski wrote:
> On Mon, 13 Jan 2020 13:45:50 +0100, Martin Schiller wrote:
>> This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2, 
>> N2 via
>> sethdlc (which needs to be patched as well).
>> 
>> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> 
>> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
>> index 5643675ff724..b28051eba736 100644
>> --- a/drivers/net/wan/hdlc_x25.c
>> +++ b/drivers/net/wan/hdlc_x25.c
>> @@ -21,8 +21,17 @@
>>  #include <linux/skbuff.h>
>>  #include <net/x25device.h>
>> 
>> +struct x25_state {
>> +	x25_hdlc_proto settings;
>> +};
>> +
>>  static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
>> 
>> +static inline struct x25_state* state(hdlc_device *hdlc)
> 
> Please no more static inlines in source files. Compiler will know what
> to do.

OK, i will remove the "inline". I've just used it, because it's also
in the hdlc_fr.c and hdlc_cisco.c code.

> 
>> +{
>> +	return (struct x25_state *)hdlc->state;
>> +}
>> +
>>  /* These functions are callbacks called by LAPB layer */
>> 
>>  static void x25_connect_disconnect(struct net_device *dev, int 
>> reason, int code)
> 
>> @@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
>> 
>>  static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
>>  {
>> +	x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
>> +	const size_t size = sizeof(x25_hdlc_proto);
>> +	x25_hdlc_proto new_settings;
>>  	hdlc_device *hdlc = dev_to_hdlc(dev);
>>  	int result;
>> 
>> @@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev, 
>> struct ifreq *ifr)
>>  		if (dev_to_hdlc(dev)->proto != &proto)
>>  			return -EINVAL;
>>  		ifr->ifr_settings.type = IF_PROTO_X25;
>> -		return 0; /* return protocol only, no settable parameters */
>> +		if (ifr->ifr_settings.size < size) {
>> +			ifr->ifr_settings.size = size; /* data size wanted */
>> +			return -ENOBUFS;
>> +		}
>> +		if (copy_to_user(x25_s, &state(hdlc)->settings, size))
>> +			return -EFAULT;
>> +		return 0;
>> 
>>  	case IF_PROTO_X25:
>>  		if (!capable(CAP_NET_ADMIN))
>> @@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev, 
>> struct ifreq *ifr)
>>  		if (dev->flags & IFF_UP)
>>  			return -EBUSY;
>> 
>> +		if (copy_from_user(&new_settings, x25_s, size))
>> +			return -EFAULT;
>> +
>> +		if ((new_settings.dce != 0 &&
>> +		     new_settings.dce != 1) ||
>> +		    (new_settings.modulo != 8 &&
>> +		     new_settings.modulo != 128) ||
>> +		    new_settings.window < 1 ||
>> +		    (new_settings.modulo == 8 &&
>> +		     new_settings.window > 7) ||
>> +		    (new_settings.modulo == 128 &&
>> +		     new_settings.window > 127) ||
>> +		    new_settings.t1 < 1 ||
>> +		    new_settings.t1 > 255 ||
>> +		    new_settings.t2 < 1 ||
>> +		    new_settings.t2 > 255 ||
>> +		    new_settings.n2 < 1 ||
>> +		    new_settings.n2 > 255)
>> +			return -EINVAL;
>> +
>>  		result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
>>  		if (result)
>>  			return result;
>> 
>> -		if ((result = attach_hdlc_protocol(dev, &proto, 0)))
>> +		if ((result = attach_hdlc_protocol(dev, &proto,
>> +						   sizeof(struct x25_state))))
>>  			return result;
>> +
>> +		memcpy(&state(hdlc)->settings, &new_settings, size);
>>  		dev->type = ARPHRD_X25;
>>  		call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
>>  		netif_dormant_off(dev);
>> diff --git a/include/uapi/linux/hdlc/ioctl.h 
>> b/include/uapi/linux/hdlc/ioctl.h
>> index 0fe4238e8246..3656ce8b8af0 100644
>> --- a/include/uapi/linux/hdlc/ioctl.h
>> +++ b/include/uapi/linux/hdlc/ioctl.h
>> @@ -3,7 +3,7 @@
>>  #define __HDLC_IOCTL_H__
>> 
>> 
>> -#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc 
>> utility */
>> +#define GENERIC_HDLC_VERSION 5	/* For synchronization with sethdlc 
>> utility */
> 
> What's the backward compatibility story in this code?

Well, I thought I have to increment the version to keep the kernel code
and the sethdlc utility in sync (like the comment says).

> 
> The IOCTL handling at least looks like it may start returning errors
> to existing user space which could have expected the parameters to
> IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.

I could also try to implement it without incrementing the version by
looking at ifr_settings.size and using the former defaults if the size
doesn't match.


> 
>>  #define CLOCK_DEFAULT   0	/* Default setting */
>>  #define CLOCK_EXT	1	/* External TX and RX clock - DTE */


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] wan/hdlc_x25: fix skb handling
  2020-01-13 13:44   ` Jakub Kicinski
@ 2020-01-14  8:10     ` Martin Schiller
  0 siblings, 0 replies; 10+ messages in thread
From: Martin Schiller @ 2020-01-14  8:10 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On 2020-01-13 14:44, Jakub Kicinski wrote:
> On Mon, 13 Jan 2020 13:45:51 +0100, Martin Schiller wrote:
>>  o call skb_reset_network_header() before hdlc->xmit()
>>  o change skb proto to HDLC (0x0019) before hdlc->xmit()
>>  o call dev_queue_xmit_nit() before hdlc->xmit()
>> 
>> This changes make it possible to trace (tcpdump) outgoing layer2
>> (ETH_P_HDLC) packets
>> 
>>  o use a copy of the skb for lapb_data_request() in x25_xmit()
> 
> It's not clear to me why

Well, this patch is ported form an older environment which is based on
linux-3.4. I can't reproduce the misbehavior with actual version, so I
will drop this part of the patch.

> 
>> This fixes the problem, that tracing layer3 (ETH_P_X25) packets
>> results in a malformed first byte of the packets.
>> 
>> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
>> ---
>>  drivers/net/wan/hdlc_x25.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
>> index b28051eba736..434e5263eddf 100644
>> --- a/drivers/net/wan/hdlc_x25.c
>> +++ b/drivers/net/wan/hdlc_x25.c
>> @@ -72,6 +72,7 @@ static int x25_data_indication(struct net_device 
>> *dev, struct sk_buff *skb)
>>  	unsigned char *ptr;
>> 
>>  	skb_push(skb, 1);
>> +	skb_reset_network_header(skb);
>> 
>>  	if (skb_cow(skb, 1))
> 
> This skb_cow() here is for the next handler down to have a 1 byte of
> headroom guaranteed? It'd seem more natural to have skb_cow before the
> push.. not that it's related to your patch.

Thanks for the hint. I will move the skb_cow() before the skb_push().

> 
>>  		return NET_RX_DROP;
>> @@ -88,6 +89,9 @@ static int x25_data_indication(struct net_device 
>> *dev, struct sk_buff *skb)
>>  static void x25_data_transmit(struct net_device *dev, struct sk_buff 
>> *skb)
>>  {
>>  	hdlc_device *hdlc = dev_to_hdlc(dev);
> 
> Please insert a new line after the variable declaration since you're
> touching this one.

OK, will do.

> 
>> +	skb_reset_network_header(skb);
>> +	skb->protocol = hdlc_type_trans(skb, dev);


I will also insert an "if (dev_nit_active(dev))" here.

>> +	dev_queue_xmit_nit(skb, dev);
>>  	hdlc->xmit(skb, dev); /* Ignore return value :-( */
>>  }
>> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
  2020-01-14  5:37   ` Martin Schiller
@ 2020-01-14 12:51     ` Jakub Kicinski
  2020-01-14 13:33       ` Martin Schiller
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-01-14 12:51 UTC (permalink / raw)
  To: Martin Schiller; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote:
> >> diff --git a/include/uapi/linux/hdlc/ioctl.h 
> >> b/include/uapi/linux/hdlc/ioctl.h
> >> index 0fe4238e8246..3656ce8b8af0 100644
> >> --- a/include/uapi/linux/hdlc/ioctl.h
> >> +++ b/include/uapi/linux/hdlc/ioctl.h
> >> @@ -3,7 +3,7 @@
> >>  #define __HDLC_IOCTL_H__
> >> 
> >> 
> >> -#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc 
> >> utility */
> >> +#define GENERIC_HDLC_VERSION 5	/* For synchronization with sethdlc 
> >> utility */  
> > 
> > What's the backward compatibility story in this code?  
> 
> Well, I thought I have to increment the version to keep the kernel code
> and the sethdlc utility in sync (like the comment says).

Perhaps I chose the wrong place for asking this question, IOCTL code
was my real worry. I don't think this version number is validated so 
I think bumping it shouldn't break anything?

> > The IOCTL handling at least looks like it may start returning errors
> > to existing user space which could have expected the parameters to
> > IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.  
> 
> I could also try to implement it without incrementing the version by
> looking at ifr_settings.size and using the former defaults if the size
> doesn't match.

Sounds good, thank you!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
  2020-01-14 12:51     ` Jakub Kicinski
@ 2020-01-14 13:33       ` Martin Schiller
  2020-01-14 13:45         ` Jakub Kicinski
  2020-03-26  6:08         ` Krzysztof Hałasa
  0 siblings, 2 replies; 10+ messages in thread
From: Martin Schiller @ 2020-01-14 13:33 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On 2020-01-14 13:51, Jakub Kicinski wrote:
> On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote:
>> >> diff --git a/include/uapi/linux/hdlc/ioctl.h
>> >> b/include/uapi/linux/hdlc/ioctl.h
>> >> index 0fe4238e8246..3656ce8b8af0 100644
>> >> --- a/include/uapi/linux/hdlc/ioctl.h
>> >> +++ b/include/uapi/linux/hdlc/ioctl.h
>> >> @@ -3,7 +3,7 @@
>> >>  #define __HDLC_IOCTL_H__
>> >>
>> >>
>> >> -#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc
>> >> utility */
>> >> +#define GENERIC_HDLC_VERSION 5	/* For synchronization with sethdlc
>> >> utility */
>> >
>> > What's the backward compatibility story in this code?
>> 
>> Well, I thought I have to increment the version to keep the kernel 
>> code
>> and the sethdlc utility in sync (like the comment says).
> 
> Perhaps I chose the wrong place for asking this question, IOCTL code
> was my real worry. I don't think this version number is validated so
> I think bumping it shouldn't break anything?

sethdlc validates the GENERIC_HDLC_VERSION at compile time.

https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/

Another question:
Where do I have to send my patch for sethdlc to?

> 
>> > The IOCTL handling at least looks like it may start returning errors
>> > to existing user space which could have expected the parameters to
>> > IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.
>> 
>> I could also try to implement it without incrementing the version by
>> looking at ifr_settings.size and using the former defaults if the size
>> doesn't match.
> 
> Sounds good, thank you!

OK, I will send a v2 of the patch.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
  2020-01-14 13:33       ` Martin Schiller
@ 2020-01-14 13:45         ` Jakub Kicinski
  2020-03-26  6:08         ` Krzysztof Hałasa
  1 sibling, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2020-01-14 13:45 UTC (permalink / raw)
  To: Martin Schiller; +Cc: khc, davem, linux-x25, netdev, linux-kernel

On Tue, 14 Jan 2020 14:33:51 +0100, Martin Schiller wrote:
> On 2020-01-14 13:51, Jakub Kicinski wrote:
> > On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote:  
> >> >> diff --git a/include/uapi/linux/hdlc/ioctl.h
> >> >> b/include/uapi/linux/hdlc/ioctl.h
> >> >> index 0fe4238e8246..3656ce8b8af0 100644
> >> >> --- a/include/uapi/linux/hdlc/ioctl.h
> >> >> +++ b/include/uapi/linux/hdlc/ioctl.h
> >> >> @@ -3,7 +3,7 @@
> >> >>  #define __HDLC_IOCTL_H__
> >> >>
> >> >>
> >> >> -#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc
> >> >> utility */
> >> >> +#define GENERIC_HDLC_VERSION 5	/* For synchronization with sethdlc
> >> >> utility */  
> >> >
> >> > What's the backward compatibility story in this code?  
> >> 
> >> Well, I thought I have to increment the version to keep the kernel 
> >> code
> >> and the sethdlc utility in sync (like the comment says).  
> > 
> > Perhaps I chose the wrong place for asking this question, IOCTL code
> > was my real worry. I don't think this version number is validated so
> > I think bumping it shouldn't break anything?  
> 
> sethdlc validates the GENERIC_HDLC_VERSION at compile time.
>
> https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/

Aw, okay, best not to bump it then.
 
> Another question:
> Where do I have to send my patch for sethdlc to?

No idea :)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
  2020-01-14 13:33       ` Martin Schiller
  2020-01-14 13:45         ` Jakub Kicinski
@ 2020-03-26  6:08         ` Krzysztof Hałasa
  1 sibling, 0 replies; 10+ messages in thread
From: Krzysztof Hałasa @ 2020-03-26  6:08 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Jakub Kicinski, khc, davem, linux-x25, netdev, linux-kernel

Hi,

I overlooked this mail, sorry.

Martin Schiller <ms@dev.tdt.de> writes:

> sethdlc validates the GENERIC_HDLC_VERSION at compile time.
>
> https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/
>
> Another question:
> Where do I have to send my patch for sethdlc to?

I guess you have to put your own version somewhere. I don't have access
to hdlc directory at kernel.org anymore, this stuff (sync serial with
HDLC) is no longer in use around here.
-- 
Krzysztof Halasa

ŁUKASIEWICZ Research Network
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-03-26  6:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 12:45 [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Martin Schiller
2020-01-13 12:45 ` [PATCH 2/2] wan/hdlc_x25: fix skb handling Martin Schiller
2020-01-13 13:44   ` Jakub Kicinski
2020-01-14  8:10     ` Martin Schiller
2020-01-13 13:53 ` [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Jakub Kicinski
2020-01-14  5:37   ` Martin Schiller
2020-01-14 12:51     ` Jakub Kicinski
2020-01-14 13:33       ` Martin Schiller
2020-01-14 13:45         ` Jakub Kicinski
2020-03-26  6:08         ` Krzysztof Hałasa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).