[PATCH 1/4] net/x25: call skb_reset_network_header where needed

[PATCH 1/4] net/x25: call skb_reset_network_header where needed

[Martin Schiller]

This fixes some problems like:
 o tcpdump not showing X.25<->LapB Header
 o "protocol 0508 is buggy" warning from I4L

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 net/x25/x25_dev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/x25/x25_dev.c b/net/x25/x25_dev.c
index 39231237e1c3..eef0305f3e99 100644
--- a/net/x25/x25_dev.c
+++ b/net/x25/x25_dev.c
@@ -127,6 +127,7 @@ int x25_lapb_receive_frame(struct sk_buff *skb, struct net_device *dev,
 
 	case X25_IFACE_DATA:
 		skb_pull(skb, 1);
+		skb_reset_network_header(skb);
 		if (x25_receive_data(skb, nb)) {
 			x25_neigh_put(nb);
 			goto out;
@@ -171,6 +172,8 @@ void x25_establish_link(struct x25_neigh *nb)
 		return;
 	}
 
+	skb_reset_network_header(skb);
+
 	skb->protocol = htons(ETH_P_X25);
 	skb->dev      = nb->dev;
 
@@ -198,6 +201,8 @@ void x25_terminate_link(struct x25_neigh *nb)
 	ptr  = skb_put(skb, 1);
 	*ptr = X25_IFACE_DISCONNECT;
 
+	skb_reset_network_header(skb);
+
 	skb->protocol = htons(ETH_P_X25);
 	skb->dev      = nb->dev;
 	dev_queue_xmit(skb);
@@ -207,8 +212,6 @@ void x25_send_frame(struct sk_buff *skb, struct x25_neigh *nb)
 {
 	unsigned char *dptr;
 
-	skb_reset_network_header(skb);
-
 	switch (nb->dev->type) {
 	case ARPHRD_X25:
 		dptr  = skb_push(skb, 1);
@@ -225,6 +228,8 @@ void x25_send_frame(struct sk_buff *skb, struct x25_neigh *nb)
 		return;
 	}
 
+	skb_reset_network_header(skb);
+
 	skb->protocol = htons(ETH_P_X25);
 	skb->dev      = nb->dev;
 
-- 
2.11.0

[PATCH 2/4] wan/hdlc_x25: fix skb handling

[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()

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 e867638067a6..fb5ed6856be5 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -66,6 +66,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;
@@ -82,6 +83,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 :-( */
 }
 
@@ -89,16 +93,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.11.0

[PATCH 3/4] isdn/i4l/isdn_x25iface: call skb_reset_network_header

[Martin Schiller]

... after skb_push() / skb_pull().

This fixes the output of tcpdump.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 drivers/isdn/i4l/isdn_x25iface.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/isdn/i4l/isdn_x25iface.c b/drivers/isdn/i4l/isdn_x25iface.c
index 48bfbcb4a09d..ffd50fa47111 100644
--- a/drivers/isdn/i4l/isdn_x25iface.c
+++ b/drivers/isdn/i4l/isdn_x25iface.c
@@ -194,6 +194,7 @@ static int isdn_x25iface_receive(struct concap_proto *cprot, struct sk_buff *skb
 	if (((ix25_pdata_t *)(cprot->proto_data))
 	    ->state == WAN_CONNECTED) {
 		if (skb_push(skb, 1)) {
+			skb_reset_network_header(skb);
 			skb->data[0] = X25_IFACE_DATA;
 			skb->protocol = x25_type_trans(skb, cprot->net_dev);
 			netif_rx(skb);
@@ -225,6 +226,7 @@ static int isdn_x25iface_connect_ind(struct concap_proto *cprot)
 	skb = dev_alloc_skb(1);
 	if (skb) {
 		skb_put_u8(skb, X25_IFACE_CONNECT);
+		skb_reset_network_header(skb);
 		skb->protocol = x25_type_trans(skb, cprot->net_dev);
 		netif_rx(skb);
 		return 0;
@@ -254,6 +256,7 @@ static int isdn_x25iface_disconn_ind(struct concap_proto *cprot)
 	skb = dev_alloc_skb(1);
 	if (skb) {
 		skb_put_u8(skb, X25_IFACE_DISCONNECT);
+		skb_reset_network_header(skb);
 		skb->protocol = x25_type_trans(skb, cprot->net_dev);
 		netif_rx(skb);
 		return 0;
@@ -278,10 +281,14 @@ static int isdn_x25iface_xmit(struct concap_proto *cprot, struct sk_buff *skb)
 	case X25_IFACE_DATA:
 		if (*state == WAN_CONNECTED) {
 			skb_pull(skb, 1);
+			skb_reset_network_header(skb);
 			netif_trans_update(cprot->net_dev);
 			ret = (cprot->dops->data_req(cprot, skb));
 			/* prepare for future retransmissions */
-			if (ret) skb_push(skb, 1);
+			if (ret) {
+				skb_push(skb, 1);
+				skb_reset_network_header(skb);
+			}
 			return ret;
 		}
 		illegal_state_warn(*state, firstbyte);
-- 
2.11.0

[PATCH 4/4] isdn/i4l: Call notifiers before and after changing device type

[Martin Schiller]

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 drivers/isdn/i4l/isdn_net.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/isdn/i4l/isdn_net.c b/drivers/isdn/i4l/isdn_net.c
index c138f66f2659..3016cbcc719a 100644
--- a/drivers/isdn/i4l/isdn_net.c
+++ b/drivers/isdn/i4l/isdn_net.c
@@ -2692,8 +2692,12 @@ isdn_net_setcfg(isdn_net_ioctl_cfg *cfg)
 			       p->dev->name);
 			return -EINVAL;
 #else
+			rtnl_lock();
+			call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE, p->dev);
 			p->dev->type = ARPHRD_PPP;	/* change ARP type */
+			call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, p->dev);
 			p->dev->addr_len = 0;
+			rtnl_unlock();
 #endif
 			break;
 		case ISDN_NET_ENCAP_X25IFACE:
@@ -2702,16 +2706,27 @@ isdn_net_setcfg(isdn_net_ioctl_cfg *cfg)
 			       p->dev->name);
 			return -EINVAL;
 #else
+			rtnl_lock();
+			call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE, p->dev);
 			p->dev->type = ARPHRD_X25;	/* change ARP type */
+			call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, p->dev);
 			p->dev->addr_len = 0;
+			rtnl_unlock();
 #endif
 			break;
 		case ISDN_NET_ENCAP_CISCOHDLCK:
 			break;
 		default:
 			if (cfg->p_encap >= 0 &&
-			    cfg->p_encap <= ISDN_NET_ENCAP_MAX_ENCAP)
+			    cfg->p_encap <= ISDN_NET_ENCAP_MAX_ENCAP) {
+				rtnl_lock();
+				call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE, p->dev);
+				p->dev->type = ARPHRD_ETHER;	/* change ARP type */
+				call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, p->dev);
+				p->dev->addr_len = ETH_ALEN;
+				rtnl_unlock();
 				break;
+			}
 			printk(KERN_WARNING
 			       "%s: encapsulation protocol %d not supported\n",
 			       p->dev->name, cfg->p_encap);
-- 
2.11.0

Re: [PATCH 2/4] wan/hdlc_x25: fix skb handling

[David Miller]

From: Martin Schiller <ms@dev.tdt.de>
Date: Wed,  3 Apr 2019 07:01:16 +0200

>  	/* 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);

This leaks 'skb'.

No way I'm applying this stuff.

Re: [PATCH 2/4] wan/hdlc_x25: fix skb handling

[Martin Schiller]

On 2019-04-05 02:32, David Miller wrote:
> From: Martin Schiller <ms@dev.tdt.de>
> Date: Wed,  3 Apr 2019 07:01:16 +0200
> 
>>  	/* 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);
> 
> This leaks 'skb'.

What exactly do you mean?
'skb' will get freed at the end of x25_xmit() function:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wan/hdlc_x25.c#n129

> 
> No way I'm applying this stuff.

Re: [PATCH 2/4] wan/hdlc_x25: fix skb handling

[David Miller]

From: Martin Schiller <ms@dev.tdt.de>
Date: Fri, 05 Apr 2019 08:56:44 +0200

> On 2019-04-05 02:32, David Miller wrote:
>> From: Martin Schiller <ms@dev.tdt.de>
>> Date: Wed,  3 Apr 2019 07:01:16 +0200
>> 
>>>  	/* 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);
>> This leaks 'skb'.
> 
> What exactly do you mean?
> 'skb' will get freed at the end of x25_xmit() function:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wan/hdlc_x25.c#n129

Then why was it freed here in the original code?

Re: [PATCH 2/4] wan/hdlc_x25: fix skb handling

[Martin Schiller]

On 2019-04-05 21:15, David Miller wrote:
> From: Martin Schiller <ms@dev.tdt.de>
> Date: Fri, 05 Apr 2019 08:56:44 +0200
> 
>> On 2019-04-05 02:32, David Miller wrote:
>>> From: Martin Schiller <ms@dev.tdt.de>
>>> Date: Wed,  3 Apr 2019 07:01:16 +0200
>>> 
>>>>  	/* 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);
>>> This leaks 'skb'.
>> 
>> What exactly do you mean?
>> 'skb' will get freed at the end of x25_xmit() function:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wan/hdlc_x25.c#n129
> 
> Then why was it freed here in the original code?

In the original code, 'skb' is only freed here if lapb_data_request()
return a value != LAPB_OK, which is the case when the skb can't be
queued for transmission. Otherwise 'skb' won't be freed here in the
"X25_IFACE_DATA" case.

What my change do is, that 'skb' is copied to 'skbn' before the skb_pull
of the first byte, to fix the problem that tracing layer3 (ETH_P_X25)
packets results in a malformed first byte of the packets, because the
original "skb" will get modified before the frame reaches the tcpdump
output.

Everything else works like before.