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 <[email protected]>
---
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
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 <[email protected]>
---
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
... after skb_push() / skb_pull().
This fixes the output of tcpdump.
Signed-off-by: Martin Schiller <[email protected]>
---
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
Signed-off-by: Martin Schiller <[email protected]>
---
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
From: Martin Schiller <[email protected]>
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.
On 2019-04-05 02:32, David Miller wrote:
> From: Martin Schiller <[email protected]>
> 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.
From: Martin Schiller <[email protected]>
Date: Fri, 05 Apr 2019 08:56:44 +0200
> On 2019-04-05 02:32, David Miller wrote:
>> From: Martin Schiller <[email protected]>
>> 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?
On 2019-04-05 21:15, David Miller wrote:
> From: Martin Schiller <[email protected]>
> Date: Fri, 05 Apr 2019 08:56:44 +0200
>
>> On 2019-04-05 02:32, David Miller wrote:
>>> From: Martin Schiller <[email protected]>
>>> 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.