[PATCH 0/8] tftp fixups

[PATCH 0/8] tftp fixups

[Enrico Scholz]

The "tftp: allocate buffers and fifo dynamically" patch in the last
patchset broke interaction with non rfc 2347 servers (e.g. when data
transfer starts immediately after RRQ/WRQ without the OACK negotiation
phase).

This has been fixed for both RRQ and WRQ requests.

For WRQ requests (push), the "tsize" option will not be sent anymore
because it is always '0' and can confuse servers.

New patches add some sanity checks which prevent modification of
internal information (blocksize or port numbers) when OACK packets
arrive in the middle of a transfer.


Enrico Scholz (8):
  tftp: make debug_assert() critical when selftest is enabled.
  tftp: remove sanity check of first block
  tftp: split out allocation and cache initialization
  tftp: accept OACK + DATA datagrams only in certain states
  tftp: support non rfc 2347 servers
  tftp: do not set 'tsize' in wrq requests
  tftp: fix WRQ support
  tftp: add some documentation about windowsize support

 Documentation/filesystems/tftp.rst |  38 ++++++
 fs/tftp.c                          | 189 +++++++++++++++++++----------
 2 files changed, 166 insertions(+), 61 deletions(-)

-- 
2.37.2

[PATCH 1/8] tftp: make debug_assert() critical when selftest is enabled.

[Enrico Scholz]

Run BUG_ON() when self test is enabled.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index e01aafce47b5..783413797251 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -84,7 +84,7 @@
 
 #define TFTP_ERR_RESEND	1
 
-#ifdef DEBUG
+#if defined(DEBUG) || IS_ENABLED(CONFIG_SELFTEST_TFTP)
 #  define debug_assert(_cond)	BUG_ON(!(_cond))
 #else
 #  define debug_assert(_cond) do {			\
-- 
2.37.2

[PATCH 2/8] tftp: remove sanity check of first block

[Enrico Scholz]

With tftp window size support, the first received block might be !=
1 (e.g. when it was reordered or dropped).  There could be checked
against '!in_window(block, 1, priv->windowsize)', but the corresponding
sanity check can be dropped completely:

- OACK logic verifies that we speak with a tftp server (which always
  sends block #1 as the first one)

- the code some lines later handles this case already

Remove the check and simplify things.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 783413797251..aaeb19590e93 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -686,14 +686,6 @@ static void tftp_recv(struct file_priv *priv,
 			priv->tftp_con->udp->uh_dport = uh_sport;
 			priv->last_block = 0;
 			tftp_window_cache_reset(&priv->cache);
-
-			if (block != 1) {	/* Assertion */
-				pr_err("error: First block is not block 1 (%d)\n",
-					block);
-				priv->err = -EINVAL;
-				priv->state = STATE_DONE;
-				break;
-			}
 		}
 
 		tftp_handle_data(priv, block, pkt + 2, len);
-- 
2.37.2

[PATCH 3/8] tftp: split out allocation and cache initialization

[Enrico Scholz]

Refactors existing code in a new function.

It also updates 'priv->state' now in error state.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index aaeb19590e93..610483d23c40 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -619,6 +619,31 @@ static void tftp_handle_data(struct file_priv *priv, uint16_t block,
 	}
 }
 
+static int tftp_allocate_transfer(struct file_priv *priv)
+{
+	debug_assert(!priv->fifo);
+
+	/* multiplication is safe; both operands were checked in tftp_parse_oack()
+	   and are small integers */
+	priv->fifo = kfifo_alloc(priv->blocksize * priv->windowsize);
+	if (!priv->fifo)
+		return -ENOMEM;
+
+	if (priv->push) {
+		priv->buf = xmalloc(priv->blocksize);
+		if (!priv->buf) {
+			kfifo_free(priv->fifo);
+			priv->fifo = NULL;
+			return -ENOMEM;
+		}
+	} else {
+		tftp_window_cache_init(&priv->cache,
+				       priv->blocksize, priv->windowsize);
+	}
+
+	return 0;
+}
+
 static void tftp_recv(struct file_priv *priv,
 			uint8_t *pkt, unsigned len, uint16_t uh_sport)
 {
@@ -723,22 +748,13 @@ static void tftp_handler(void *ctx, char *packet, unsigned len)
 
 static int tftp_start_transfer(struct file_priv *priv)
 {
-	/* multiplication is safe; both operands where checked in tftp_parse_oack()
-	   and are small integers */
-	priv->fifo = kfifo_alloc(priv->blocksize * priv->windowsize);
-	if (!priv->fifo)
-		return -ENOMEM;
+	int rc;
 
-	if (priv->push) {
-		priv->buf = xmalloc(priv->blocksize);
-		if (!priv->buf) {
-			kfifo_free(priv->fifo);
-			priv->fifo = NULL;
-			return -ENOMEM;
-		}
-	} else {
-		tftp_window_cache_init(&priv->cache,
-				       priv->blocksize, priv->windowsize);
+	rc = tftp_allocate_transfer(priv);
+	if (rc < 0) {
+		priv->err = rc;
+		priv->state = STATE_DONE;
+		return rc;
 	}
 
 	if (priv->push) {
-- 
2.37.2

[PATCH 4/8] tftp: accept OACK + DATA datagrams only in certain states

[Enrico Scholz]

These packets are valid in certain points of the transfer only and
accepting them too early or too late can corrupt internal states.

Reject them when they are unexpected.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/tftp.c b/fs/tftp.c
index 610483d23c40..fb6c368b3a64 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -690,6 +690,12 @@ static void tftp_recv(struct file_priv *priv,
 		break;
 
 	case TFTP_OACK:
+		if (priv->state != STATE_RRQ && priv->state != STATE_WRQ) {
+			pr_warn("OACK packet in %s state\n",
+				tftp_states[priv->state]);
+			break;
+		}
+
 		priv->tftp_con->udp->uh_dport = uh_sport;
 
 		if (tftp_parse_oack(priv, pkt, len) < 0) {
@@ -713,6 +719,12 @@ static void tftp_recv(struct file_priv *priv,
 			tftp_window_cache_reset(&priv->cache);
 		}
 
+		if (priv->state != STATE_RDATA) {
+			pr_warn("DATA packet in %s state\n",
+				tftp_states[priv->state]);
+			break;
+		}
+
 		tftp_handle_data(priv, block, pkt + 2, len);
 
 		break;
-- 
2.37.2

[PATCH 5/8] tftp: support non rfc 2347 servers

[Enrico Scholz]

State machine must be changed for servers without OACK support.  Here,
objects for data transfer must be allocated before the first datagram
is handled and it is possible that whole transfer finishes at this
point.

Fixes "tftp: allocate buffers and fifo dynamically"

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 87 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 58 insertions(+), 29 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index fb6c368b3a64..7edea904aabe 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -64,13 +64,12 @@
 #define STATE_WRQ	2
 #define STATE_RDATA	3
 #define STATE_WDATA	4
-#define STATE_OACK	5
+/* OACK from server has been received and we can begin to sent either the ACK
+   (for RRQ) or data (for WRQ) */
+#define STATE_START	5
 #define STATE_WAITACK	6
 #define STATE_LAST	7
 #define STATE_DONE	8
-/* OACK from server has been received and we can begin to sent either the ACK
-   (for RRQ) or data (for WRQ) */
-#define STATE_START	9
 
 #define TFTP_BLOCK_SIZE		512	/* default TFTP block size */
 #define TFTP_MTU_SIZE		1432	/* MTU based block size */
@@ -363,7 +362,6 @@ static char const * const tftp_states[] = {
 	[STATE_WRQ] = "WRQ",
 	[STATE_RDATA] = "RDATA",
 	[STATE_WDATA] = "WDATA",
-	[STATE_OACK] = "OACK",
 	[STATE_WAITACK] = "WAITACK",
 	[STATE_LAST] = "LAST",
 	[STATE_DONE] = "DONE",
@@ -431,7 +429,6 @@ static int tftp_send(struct file_priv *priv)
 		break;
 
 	case STATE_RDATA:
-	case STATE_OACK:
 		xp = pkt;
 		s = (uint16_t *)pkt;
 		*s++ = htons(TFTP_ACK);
@@ -649,6 +646,7 @@ static void tftp_recv(struct file_priv *priv,
 {
 	uint16_t opcode;
 	uint16_t block;
+	int rc;
 
 	/* according to RFC1350 minimal tftp packet length is 4 bytes */
 	if (len < 4)
@@ -711,12 +709,20 @@ static void tftp_recv(struct file_priv *priv,
 		len -= 2;
 		block = ntohs(*(uint16_t *)pkt);
 
-		if (priv->state == STATE_RRQ || priv->state == STATE_OACK) {
-			/* first block received */
-			priv->state = STATE_RDATA;
+		if (priv->state == STATE_RRQ) {
+			/* first block received; entered only with non rfc
+			   2347 (TFTP Option extension) compliant servers */
 			priv->tftp_con->udp->uh_dport = uh_sport;
+			priv->state = STATE_RDATA;
 			priv->last_block = 0;
-			tftp_window_cache_reset(&priv->cache);
+			priv->ack_block = priv->windowsize;
+
+			rc = tftp_allocate_transfer(priv);
+			if (rc < 0) {
+				priv->err = rc;
+				priv->state = STATE_DONE;
+				break;
+			}
 		}
 
 		if (priv->state != STATE_RDATA) {
@@ -726,7 +732,6 @@ static void tftp_recv(struct file_priv *priv,
 		}
 
 		tftp_handle_data(priv, block, pkt + 2, len);
-
 		break;
 
 	case TFTP_ERROR:
@@ -775,7 +780,7 @@ static int tftp_start_transfer(struct file_priv *priv)
 		priv->block = 1;
 	} else {
 		/* send ACK */
-		priv->state = STATE_OACK;
+		priv->state = STATE_RDATA;
 		priv->last_block = 0;
 		tftp_send(priv);
 	}
@@ -828,26 +833,49 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 		goto out1;
 
 	tftp_timer_reset(priv);
-	while (priv->state != STATE_DONE && priv->state != STATE_START) {
-		ret = tftp_poll(priv);
-		if (ret == TFTP_ERR_RESEND)
-			tftp_send(priv);
-		if (ret < 0)
-			goto out1;
-	}
 
-	if (priv->state == STATE_DONE) {
-		/* this should not happen; STATE_DONE without error happens
-		   after completing the transfer but this has not been started
-		   yet */
-		if (WARN_ON(priv->err == 0))
-			priv->err = -EIO;
+	/* - 'ret < 0'  ... error
+	   - 'ret == 0' ... further tftp_poll() required
+	   - 'ret == 1' ... startup finished */
+	do {
+		switch (priv->state) {
+		case STATE_DONE:
+			/* branch is entered in two situations:
+			   - non rfc 2347 compliant servers finished the
+			     transfer by sending a small file
+			   - some error occurred */
+			if (priv->err < 0)
+				ret = priv->err;
+			else
+				ret = 1;
+			break;
 
-		ret = priv->err;
-		goto out1;
-	}
+		case STATE_START:
+			ret = tftp_start_transfer(priv);
+			if (!ret)
+				ret = 1;
+			break;
+
+		case STATE_RDATA:
+			/* first data block of non rfc 2347 servers */
+			ret = 1;
+			break;
+
+		case STATE_RRQ:
+		case STATE_WRQ:
+			ret = tftp_poll(priv);
+			if (ret == TFTP_ERR_RESEND) {
+				tftp_send(priv);
+				ret = 0;
+			}
+			break;
+
+		default:
+			debug_assert(false);
+			break;
+		}
+	} while (ret == 0);
 
-	ret = tftp_start_transfer(priv);
 	if (ret < 0)
 		goto out1;
 
@@ -855,6 +883,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 out1:
 	net_unregister(priv->tftp_con);
 out:
+	debug_assert(!priv->fifo);
 	free(priv);
 
 	return ERR_PTR(ret);
-- 
2.37.2

[PATCH 6/8] tftp: do not set 'tsize' in wrq requests

[Enrico Scholz]

The filesize is not known for push requests and barebox always sent
'0'.  Server might reject data because it will always exceed this
length.

Send this option only for rrq requests.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 7edea904aabe..c00857ecfa28 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -403,22 +403,26 @@ static int tftp_send(struct file_priv *priv)
 				"octet%c"
 				"timeout%c"
 				"%d%c"
-				"tsize%c"
-				"%lld%c"
 				"blksize%c"
 				"%u",
 				priv->filename + 1, '\0',
 				'\0',	/* "octet" */
 				'\0',	/* "timeout" */
 				TIMEOUT, '\0',
-				'\0',	/* "tsize" */
-				priv->filesize, '\0',
 				'\0',	/* "blksize" */
 				/* use only a minimal blksize for getattr
 				   operations, */
 				priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE);
 		pkt++;
 
+		if (!priv->push)
+			/* we do not know the filesize in WRQ requests and
+			   'priv->filesize' will always be zero */
+			pkt += sprintf((unsigned char *)pkt,
+				       "tsize%c%lld%c",
+				       '\0', priv->filesize,
+				       '\0');
+
 		if (window_size > 1)
 			pkt += sprintf((unsigned char *)pkt,
 				       "windowsize%c%u%c",
-- 
2.37.2

[PATCH 7/8] tftp: fix WRQ support

[Enrico Scholz]

"tftp: allocate buffers and fifo dynamically" broke WRQ support.
Reenable it.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index c00857ecfa28..174365d6ed0a 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -673,22 +673,36 @@ static void tftp_recv(struct file_priv *priv,
 		if (!priv->push)
 			break;
 
-		priv->block = ntohs(*(uint16_t *)pkt);
-		if (priv->block != priv->last_block) {
-			pr_vdebug("ack %d != %d\n", priv->block, priv->last_block);
+		block = ntohs(*(uint16_t *)pkt);
+		if (block != priv->last_block) {
+			pr_vdebug("ack %d != %d\n", block, priv->last_block);
 			break;
 		}
 
-		priv->block++;
+		switch (priv->state) {
+		case STATE_WRQ:
+			priv->tftp_con->udp->uh_dport = uh_sport;
+			priv->state = STATE_START;
+			break;
 
-		tftp_timer_reset(priv);
+		case STATE_WAITACK:
+			priv->state = STATE_WDATA;
+			break;
 
-		if (priv->state == STATE_LAST) {
+		case STATE_LAST:
 			priv->state = STATE_DONE;
 			break;
+
+		default:
+			pr_warn("ACK packet in %s state\n",
+				tftp_states[priv->state]);
+			goto ack_out;
 		}
-		priv->tftp_con->udp->uh_dport = uh_sport;
-		priv->state = STATE_WDATA;
+
+		priv->block = block + 1;
+		tftp_timer_reset(priv);
+
+	ack_out:
 		break;
 
 	case TFTP_OACK:
-- 
2.37.2

[PATCH 8/8] tftp: add some documentation about windowsize support

[Enrico Scholz]

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 Documentation/filesystems/tftp.rst | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/filesystems/tftp.rst b/Documentation/filesystems/tftp.rst
index a292765e2511..8929213d3c4a 100644
--- a/Documentation/filesystems/tftp.rst
+++ b/Documentation/filesystems/tftp.rst
@@ -20,3 +20,41 @@ Example:
 
 In addition to the TFTP filesystem implementation, barebox does also have a
 :ref:`tftp command <command_tftp>`.
+
+RFC 7440 "windowsize" support
+=============================
+
+barebox supports the tftp windowsize option for downloading files.  It
+is not implemented for uploads.
+
+Generally, this option greatly improves the download speed (factors
+4-30 are not uncommon).  But choosing a too large windowsize can have
+the opposite effect.  Performance depends on:
+
+ - the network infrastructure: when the tftp server sends files with
+   1Gb/s but there are components in the network (switches or the
+   target nic) which support only 100 Mb/s, packets will be dropped.
+
+   The lower the internal buffers of the bottleneck components, the
+   lower the optimal window size.
+
+   In practice (iMX8MP on a Netgear GS108Ev3 with a port configured to
+   100 Mb/s) it had to be reduced to
+
+   .. code-block:: console
+     global tftp.windowsize=26
+
+   for example.
+
+ - the target network driver: datagrams from server will arive faster
+   than they can be processed and must be buffered internally.  For
+   example, the `fec-imx` driver reserves place for
+
+   .. code-block:: c
+     #define FEC_RBD_NUM		64
+
+   packets before they are dropped
+
+ - partially the workload: copying downloaded files to ram will be
+   faster than burning them into flash.  Latter can consume internal
+   buffers quicker so that windowsize might be reduced
-- 
2.37.2

Re: [PATCH 0/8] tftp fixups

[Ahmad Fatoum]

Hello Enrico,

On 28.08.22 16:02, Enrico Scholz wrote:
> The "tftp: allocate buffers and fifo dynamically" patch in the last
> patchset broke interaction with non rfc 2347 servers (e.g. when data
> transfer starts immediately after RRQ/WRQ without the OACK negotiation
> phase).
> 
> This has been fixed for both RRQ and WRQ requests.
> 
> For WRQ requests (push), the "tsize" option will not be sent anymore
> because it is always '0' and can confuse servers.
> 
> New patches add some sanity checks which prevent modification of
> internal information (blocksize or port numbers) when OACK packets
> arrive in the middle of a transfer.

The patch introducing the regression is still in next, which can be
rebased unlike master. Perhaps it would be best you provide a fixup
for the commit in question (or a revised series, if it would cause
conflicts for later commits)? That way, we don't end up with breakage
when doing a bisect in future.

The remaining sanity checking in this series could then be stacked
on top.

Cheers,
Ahmad

> 
> 
> Enrico Scholz (8):
>   tftp: make debug_assert() critical when selftest is enabled.
>   tftp: remove sanity check of first block
>   tftp: split out allocation and cache initialization
>   tftp: accept OACK + DATA datagrams only in certain states
>   tftp: support non rfc 2347 servers
>   tftp: do not set 'tsize' in wrq requests
>   tftp: fix WRQ support
>   tftp: add some documentation about windowsize support
> 
>  Documentation/filesystems/tftp.rst |  38 ++++++
>  fs/tftp.c                          | 189 +++++++++++++++++++----------
>  2 files changed, 166 insertions(+), 61 deletions(-)
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/8] tftp fixups

[Sascha Hauer]

On Mon, Aug 29, 2022 at 12:52:16PM +0200, Ahmad Fatoum wrote:
> Hello Enrico,
> 
> On 28.08.22 16:02, Enrico Scholz wrote:
> > The "tftp: allocate buffers and fifo dynamically" patch in the last
> > patchset broke interaction with non rfc 2347 servers (e.g. when data
> > transfer starts immediately after RRQ/WRQ without the OACK negotiation
> > phase).
> > 
> > This has been fixed for both RRQ and WRQ requests.
> > 
> > For WRQ requests (push), the "tsize" option will not be sent anymore
> > because it is always '0' and can confuse servers.
> > 
> > New patches add some sanity checks which prevent modification of
> > internal information (blocksize or port numbers) when OACK packets
> > arrive in the middle of a transfer.
> 
> The patch introducing the regression is still in next, which can be
> rebased unlike master. Perhaps it would be best you provide a fixup
> for the commit in question (or a revised series, if it would cause
> conflicts for later commits)? That way, we don't end up with breakage
> when doing a bisect in future.

Yes, please. Both a fixup patch or a revised series would be fine with
me.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |