diff mbox series

drivers/net/wan/x25_asy: Fix to make it work

Message ID 20200708043754.46554-1-xie.he.0141@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series drivers/net/wan/x25_asy: Fix to make it work | expand

Commit Message

Xie He July 8, 2020, 4:37 a.m. UTC 
This driver is not working because of problems of its receiving code.
This patch fixes it to make it work.

When the driver receives an LAPB frame, it should first pass the frame
to the LAPB module to process. After processing, the LAPB module passes
the data (the packet) back to the driver, the driver should then add a
one-byte pseudo header and pass the data to upper layers.

The changes to the "x25_asy_bump" function and the
"x25_asy_data_indication" function are to correctly implement this
procedure.

Also, the "x25_asy_unesc" function ignores any frame that is shorter
than 3 bytes. However the shortest frames are 2-byte long. So we need
to change it to allow 2-byte frames to pass.

Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/x25_asy.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

David Miller July 8, 2020, 5:13 p.m. UTC | #1 
From: Xie He <xie.he.0141@gmail.com>
Date: Tue,  7 Jul 2020 21:37:54 -0700

> This driver is not working because of problems of its receiving code.
> This patch fixes it to make it work.
> 
> When the driver receives an LAPB frame, it should first pass the frame
> to the LAPB module to process. After processing, the LAPB module passes
> the data (the packet) back to the driver, the driver should then add a
> one-byte pseudo header and pass the data to upper layers.
> 
> The changes to the "x25_asy_bump" function and the
> "x25_asy_data_indication" function are to correctly implement this
> procedure.
> 
> Also, the "x25_asy_unesc" function ignores any frame that is shorter
> than 3 bytes. However the shortest frames are 2-byte long. So we need
> to change it to allow 2-byte frames to pass.
> 
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Something's not right, because I find it hard to believe this has been
so fundamentally broken for such a long period of time.

Maybe the drivers all handle things differently, and whilst your change
might fix some drivers, it will break others.

I'm not applying this until this situation is better understood.
Xie He July 8, 2020, 7:04 p.m. UTC | #2 
From: David Miller <davem@davemloft.net>
Date: Wed, Jul 8, 2020 at 10:13 AM -0700
> Something's not right, because I find it hard to believe this has been
> so fundamentally broken for such a long period of time.
>
> Maybe the drivers all handle things differently, and whilst your change
> might fix some drivers, it will break others.
>
> I'm not applying this until this situation is better understood.

Yes, it was hard for me to believe, too.

At first when I tried this driver, it was silently not able to establish
LAPB connections, I found that it was because it was ignoring
2-byte frames. I changed it to make 2-byte frames pass. Then I
encountered kernel panic. I don't know how to solve it, so I looked
at the way "lapbether" does things and changed this driver according
to the "lapbether" driver. And it worked.

The "lapbether" driver and this driver both use the "lapb" module to
implement the LAPB protocol, so they should implement LAPB-related
code in the same way.

This patch only changes this driver and does not affect other drivers.

I don't know how I can better explain this situation. Please tell me
anything I can do to help. Thanks!
Xie He July 9, 2020, 4:15 a.m. UTC | #3 
This email is a detailed explanation of how to test the LAPB drivers,
just in case you have time to check. Thanks!

This email has 4 parts.
  1) How to set up "lapbether" links (for comparison)
  2) How to set up "x25_asy" links
  3) How to test using AF_X25 sockets
  4) How to test using AF_PACKET sockets (for simpler debugging)

You can compare the behavior of "lapbether" and "x25_asy".
You can also compare the behavior of "x25_asy" before and after
my change.

For the C code in this email, I'm sorry that for brevity, I didn't
include error checking. Declarations and statements are also mixed
which doesn't conform to the style in the Linux kernel. I won't
produce this kind of code when I am actually doing programming.

If you have any issue using or understanding my code. Please feel free
to ask me. Thanks!

--------------------------------------------------------
1) How to set up "lapbether" links

First set up a virtual Ethernet link:
  sudo ip link add veth1 type veth peer name veth0
  sudo ip link set veth0 up
  sudo ip link set veth1 up

Then:
  sudo modprobe lapb
  sudo modprobe lapbether

The lapbether driver will set up an LAPB interface for each Ethernet
interface, named lapb0, lapb1, lapb2, ...

Find the LAPB interfaces corresponding to veth0 and veth1, and use
  sudo ip link set lapbN up
to bring them up.

--------------------------------------------------------
2) How to set up "x25_asy" links

First:
  sudo modprobe lapb
  sudo modprobe x25_asy

Then set up a virtual TTY link:
  socat -d -d pty,cfmakeraw pty,cfmakeraw &
This will open a pair of PTY ports.
(The "socat" program can be installed from package managers.)

Then use a C program to set the line discipline for the two PTY ports:
  int ldisc = N_X25;
  int fd = open("path/to/pty", O_RDWR);
  ioctl(fd, TIOCSETD, &ldisc);
  close(fd);
Then we'll get two network interfaces named x25asy0 and x25asy1.

Then we do:
  sudo ip link set x25asyN up
to bring them up.

--------------------------------------------------------
3) How to test using AF_X25 sockets

Note that don't test a "lapbether" link and a "x25_asy" link at the
same time using AF_X25 sockets. There would be a kernel panic because
of bugs in the x25 module. I don't know how to fix this issue now but
may be able to in the future.

First:
  sudo modprobe x25

Then set up the routing table:
  sudo route -A x25 add 1/1 lapb1
or
  sudo route -A x25 add 1/1 x25asy0

Then in the server C program:
  int sockfd = socket(AF_X25, SOCK_SEQPACKET, 0);

  /* Bind local address */
  struct sockaddr_x25 serv_addr = {
      .sx25_family = AF_X25,
  };
  strcpy(serv_addr.sx25_addr.x25_addr, "111"); /* 111: server addr */
  bind(sockfd, (struct sockaddr *)&serv_addr, sizeof serv_addr);

  /* Wait for connections */
  listen(sockfd, 5);

  /* Accept connection */
  struct sockaddr_x25 client_addr;
  socklen_t client_addr_len = sizeof client_addr;
  int connected_sockfd = accept(sockfd, (struct sockaddr *)&client_addr,
                                &client_addr_len);

  char buffer[1000];
  ssize_t length = recv(connected_sockfd, buffer, sizeof buffer, 0);

  close(connected_sockfd);
  close(sockfd);

And in the client C program:
  int sockfd = socket(AF_X25, SOCK_SEQPACKET, 0);

  /* Bind local address */
  struct sockaddr_x25 local_addr = {
      .sx25_family = AF_X25,
  };
  strcpy(local_addr.sx25_addr.x25_addr, "777"); /* 777: local addr */
  bind(sockfd, (struct sockaddr *)&local_addr, sizeof local_addr);

  /* Connect to the server */
  struct sockaddr_x25 serv_addr = {
      .sx25_family = AF_X25,
  };
  strcpy(serv_addr.sx25_addr.x25_addr, "111"); /* 111: server addr */
  connect(sockfd, (struct sockaddr *)&serv_addr, sizeof serv_addr);

  send(sockfd, "data", 4, MSG_EOR);

  usleep(10000); /* Wait a while before closing */

  close(sockfd);

--------------------------------------------------------
4) How to test using AF_PACKET sockets

In the connected-side C program:
  int sockfd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_ALL));

  /* Get interface index */
  struct ifreq ifr;
  strcpy(ifr.ifr_name, "interface_name");
  ioctl(sockfd, SIOCGIFINDEX, &ifr);
  int ifindex = ifr.ifr_ifindex;

  struct sockaddr_ll sender_addr;
  socklen_t sender_addr_len = sizeof sender_addr;
  char buffer[1500];

  while (1) {
      ssize_t length = recvfrom(sockfd, buffer, sizeof buffer, 0,
                                (struct sockaddr *)&sender_addr,
                                &sender_addr_len);
      if (sender_addr.sll_ifindex != ifindex)
          continue;
      else if (buffer[0] == 0)
          printf("Data received.\n");
      else if (buffer[0] == 1)
          printf("Connected by the other side.\n");
      else if (buffer[0] == 2) {
          printf("Disconnected by the other side.\n");
          break;
      }
  }

  close(sockfd);

In the connecting-side C program:
  int sockfd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_ALL));

  /* Get interface index */
  struct ifreq ifr;
  strcpy(ifr.ifr_name, "interface_name");
  ioctl(sockfd, SIOCGIFINDEX, &ifr);
  int ifindex = ifr.ifr_ifindex;

  struct sockaddr_ll addr = {
      .sll_family = AF_PACKET,
      .sll_ifindex = ifindex,
  };

  /* Connect */
  sendto(sockfd, "\x01", 1, 0, (struct sockaddr *)&addr, sizeof addr);

  /* Send data */
  sendto(sockfd, "\x00" "data", 5, 0, (struct sockaddr *)&addr,
         sizeof addr);

  sleep(1); /* Wait a while before disconnecting */

  /* Disconnect */
  sendto(sockfd, "\x02", 1, 0, (struct sockaddr *)&addr, sizeof addr);

  close(sockfd);
Eric Dumazet July 13, 2020, 9:21 p.m. UTC | #4 
On 7/7/20 9:37 PM, Xie He wrote:
> This driver is not working because of problems of its receiving code.
> This patch fixes it to make it work.
> 
> When the driver receives an LAPB frame, it should first pass the frame
> to the LAPB module to process. After processing, the LAPB module passes
> the data (the packet) back to the driver, the driver should then add a
> one-byte pseudo header and pass the data to upper layers.
> 
> The changes to the "x25_asy_bump" function and the
> "x25_asy_data_indication" function are to correctly implement this
> procedure.
> 
> Also, the "x25_asy_unesc" function ignores any frame that is shorter
> than 3 bytes. However the shortest frames are 2-byte long. So we need
> to change it to allow 2-byte frames to pass.
> 
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
>  drivers/net/wan/x25_asy.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
> index 69773d228ec1..3fd8938e591b 100644
> --- a/drivers/net/wan/x25_asy.c
> +++ b/drivers/net/wan/x25_asy.c
> @@ -183,7 +183,7 @@ static inline void x25_asy_unlock(struct x25_asy *sl)
>  	netif_wake_queue(sl->dev);
>  }
>  
> -/* Send one completely decapsulated IP datagram to the IP layer. */
> +/* Send an LAPB frame to the LAPB module to process. */
>  
>  static void x25_asy_bump(struct x25_asy *sl)
>  {
> @@ -195,13 +195,12 @@ static void x25_asy_bump(struct x25_asy *sl)
>  	count = sl->rcount;
>  	dev->stats.rx_bytes += count;
>  
> -	skb = dev_alloc_skb(count+1);
> +	skb = dev_alloc_skb(count);
>  	if (skb == NULL) {
>  		netdev_warn(sl->dev, "memory squeeze, dropping packet\n");
>  		dev->stats.rx_dropped++;
>  		return;
>  	}
> -	skb_push(skb, 1);	/* LAPB internal control */
>  	skb_put_data(skb, sl->rbuff, count);
>  	skb->protocol = x25_type_trans(skb, sl->dev);
>  	err = lapb_data_received(skb->dev, skb);
> @@ -209,7 +208,6 @@ static void x25_asy_bump(struct x25_asy *sl)
>  		kfree_skb(skb);
>  		printk(KERN_DEBUG "x25_asy: data received err - %d\n", err);
>  	} else {
> -		netif_rx(skb);
>  		dev->stats.rx_packets++;
>  	}
>  }
> @@ -356,12 +354,16 @@ static netdev_tx_t x25_asy_xmit(struct sk_buff *skb,
>   */
>  
>  /*
> - *	Called when I frame data arrives. We did the work above - throw it
> - *	at the net layer.
> + *	Called when I frame data arrives. We add a pseudo header for upper
> + *	layers and pass it to upper layers.
>   */
>  
>  static int x25_asy_data_indication(struct net_device *dev, struct sk_buff *skb)
>  {

It is not clear to me what guarantee we have to have one byte of headroom in the skb
at this point.

You might add to be safe : (as done in lapbeth_data_indication(), but after the skb_push() which seems wrong)

      if (skb_cow(skb, 1)) {
            kfree_skb(skb); /* This line I am not sure, but looking at
                             * lapb_data_indication() this might be needed.
                             */
	    return NET_RX_DROP;
      }

> +	skb_push(skb, 1);
> +	skb->data[0] = X25_IFACE_DATA;
> +	skb->protocol = x25_type_trans(skb, dev);
> +
>  	return netif_rx(skb);
>  }
>  
> @@ -657,7 +659,7 @@ static void x25_asy_unesc(struct x25_asy *sl, unsigned char s)
>  	switch (s) {
>  	case X25_END:
>  		if (!test_and_clear_bit(SLF_ERROR, &sl->flags) &&
> -		    sl->rcount > 2)
> +		    sl->rcount >= 2)
>  			x25_asy_bump(sl);
>  		clear_bit(SLF_ESCAPE, &sl->flags);
>  		sl->rcount = 0;
>
Xie He July 13, 2020, 10:19 p.m. UTC | #5 
On Mon, Jul 13, 2020 at 2:21 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> It is not clear to me what guarantee we have to have one byte of headroom in the skb
> at this point.
>
> You might add to be safe : (as done in lapbeth_data_indication(), but after the skb_push() which seems wrong)
>
>       if (skb_cow(skb, 1)) {
>             kfree_skb(skb); /* This line I am not sure, but looking at
>                              * lapb_data_indication() this might be needed.
>                              */
>             return NET_RX_DROP;
>       }
>
Thank you for your review, Eric!

The function "x25_asy_data_indication" is called by the "lapb" module
(net/lapb/). Before the "lapb" module calls this function, it has
removed from the skb an LAPB header which is at least 2 bytes (in the
function "lapb_decode"). So I thought there would always be a headroom
of one byte at this point.

But yes, it is always safer to add "skb_cow" at this point, so that it
is clearer the code would not crash here. I'll add it in the second
version of this patch. Thank you for your suggestion!

And yes, I agree that in "lapbeth_data_indication", the order of
"skb_push" and "skb_cow" is probably wrong. Let us submit another
patch to fix this problem!
Martin Schiller July 14, 2020, 8:07 a.m. UTC | #6 
On 2020-07-08 21:04, Xie He wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, Jul 8, 2020 at 10:13 AM -0700
>> Something's not right, because I find it hard to believe this has been
>> so fundamentally broken for such a long period of time.
>> 
>> Maybe the drivers all handle things differently, and whilst your 
>> change
>> might fix some drivers, it will break others.
>> 
>> I'm not applying this until this situation is better understood.
> 
> Yes, it was hard for me to believe, too.
> 
> At first when I tried this driver, it was silently not able to 
> establish
> LAPB connections, I found that it was because it was ignoring
> 2-byte frames. I changed it to make 2-byte frames pass. Then I
> encountered kernel panic. I don't know how to solve it, so I looked
> at the way "lapbether" does things and changed this driver according
> to the "lapbether" driver. And it worked.
> 
> The "lapbether" driver and this driver both use the "lapb" module to
> implement the LAPB protocol, so they should implement LAPB-related
> code in the same way.
> 
> This patch only changes this driver and does not affect other drivers.
> 
> I don't know how I can better explain this situation. Please tell me
> anything I can do to help. Thanks!

It really seems very strange that this driver seems to contain such
fundamental errors. I have never used it.

But the explanations and fixes look plausible to me.
Xie He July 14, 2020, 9:55 a.m. UTC | #7 
On Tue, Jul 14, 2020 at 1:07 AM -0700
Martin Schiller <ms@dev.tdt.de> wrote:
>
> It really seems very strange that this driver seems to contain such
> fundamental errors. I have never used it.
>
> But the explanations and fixes look plausible to me.

Thank you for reviewing this patch, Martin!

Yes, this situation is very strange. I guess not so many people have
tried this driver. The comment of this driver says it doesn't
implement the checksum, which is required by the international
standard. So I guess it can't be used for practical purposes anyway. I
tried this driver because I was personally interested in X.25.
diff mbox series

Patch

diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
index 69773d228ec1..3fd8938e591b 100644
--- a/drivers/net/wan/x25_asy.c
+++ b/drivers/net/wan/x25_asy.c
@@ -183,7 +183,7 @@  static inline void x25_asy_unlock(struct x25_asy *sl)
 	netif_wake_queue(sl->dev);
 }
 
-/* Send one completely decapsulated IP datagram to the IP layer. */
+/* Send an LAPB frame to the LAPB module to process. */
 
 static void x25_asy_bump(struct x25_asy *sl)
 {
@@ -195,13 +195,12 @@  static void x25_asy_bump(struct x25_asy *sl)
 	count = sl->rcount;
 	dev->stats.rx_bytes += count;
 
-	skb = dev_alloc_skb(count+1);
+	skb = dev_alloc_skb(count);
 	if (skb == NULL) {
 		netdev_warn(sl->dev, "memory squeeze, dropping packet\n");
 		dev->stats.rx_dropped++;
 		return;
 	}
-	skb_push(skb, 1);	/* LAPB internal control */
 	skb_put_data(skb, sl->rbuff, count);
 	skb->protocol = x25_type_trans(skb, sl->dev);
 	err = lapb_data_received(skb->dev, skb);
@@ -209,7 +208,6 @@  static void x25_asy_bump(struct x25_asy *sl)
 		kfree_skb(skb);
 		printk(KERN_DEBUG "x25_asy: data received err - %d\n", err);
 	} else {
-		netif_rx(skb);
 		dev->stats.rx_packets++;
 	}
 }
@@ -356,12 +354,16 @@  static netdev_tx_t x25_asy_xmit(struct sk_buff *skb,
  */
 
 /*
- *	Called when I frame data arrives. We did the work above - throw it
- *	at the net layer.
+ *	Called when I frame data arrives. We add a pseudo header for upper
+ *	layers and pass it to upper layers.
  */
 
 static int x25_asy_data_indication(struct net_device *dev, struct sk_buff *skb)
 {
+	skb_push(skb, 1);
+	skb->data[0] = X25_IFACE_DATA;
+	skb->protocol = x25_type_trans(skb, dev);
+
 	return netif_rx(skb);
 }
 
@@ -657,7 +659,7 @@  static void x25_asy_unesc(struct x25_asy *sl, unsigned char s)
 	switch (s) {
 	case X25_END:
 		if (!test_and_clear_bit(SLF_ERROR, &sl->flags) &&
-		    sl->rcount > 2)
+		    sl->rcount >= 2)
 			x25_asy_bump(sl);
 		clear_bit(SLF_ESCAPE, &sl->flags);
 		sl->rcount = 0;