From e5f69f82585be23fe31abadb3eb496fb215dc476 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:37:56 +0200 Subject: tftp: add some 'const' annotations Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-2-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index d186e7983a..c26204ae76 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -94,7 +94,7 @@ static int tftp_truncate(struct device_d *dev, FILE *f, loff_t size) return 0; } -static char *tftp_states[] = { +static char const * const tftp_states[] = { [STATE_INVALID] = "INVALID", [STATE_RRQ] = "RRQ", [STATE_WRQ] = "WRQ", -- cgit v1.2.1 From 2295c717eed68cde6b2caf67c12fe9ee4c449f93 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:37:57 +0200 Subject: tftp: allow to change tftp port This adds a 'port=' mount option for tftp filesystems. Useful e.g. when working with a local, non-privileged tftp server Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-3-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index c26204ae76..913ca1df6e 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #define TFTP_PORT 69 /* Well known TFTP port number */ @@ -376,6 +377,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev, struct file_priv *priv; struct tftp_priv *tpriv = dev->priv; int ret; + unsigned short port = TFTP_PORT; priv = xzalloc(sizeof(*priv)); @@ -405,8 +407,9 @@ static struct file_priv *tftp_do_open(struct device_d *dev, goto out; } - priv->tftp_con = net_udp_new(tpriv->server, TFTP_PORT, tftp_handler, - priv); + parseopt_hu(fsdev->options, "port", &port); + + priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv); if (IS_ERR(priv->tftp_con)) { ret = PTR_ERR(priv->tftp_con); goto out1; -- cgit v1.2.1 From 0e12f6eb1b3e6c6e447586c0b72edf1663c0ae96 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:37:59 +0200 Subject: tftp: do not set 'tsize' in WRQ requests 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 Link: https://lore.barebox.org/20220830073816.2694734-5-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 913ca1df6e..361661d218 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -132,18 +132,23 @@ static int tftp_send(struct file_priv *priv) "octet%c" "timeout%c" "%d%c" - "tsize%c" - "%lld%c" "blksize%c" "1432", priv->filename + 1, 0, 0, 0, TIMEOUT, 0, - 0, - priv->filesize, 0, 0); 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'); + len = pkt - xp; break; -- cgit v1.2.1 From 31d0e29568031d087b4684f875cac09b878ae8ae Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:00 +0200 Subject: tftp: assign 'priv->block' later in WRQ Some refactoring; makes next patches cleaner. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-6-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 361661d218..c1a1937117 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -254,6 +254,7 @@ static void tftp_recv(struct file_priv *priv, uint8_t *pkt, unsigned len, uint16_t uh_sport) { uint16_t opcode; + uint16_t block; /* according to RFC1350 minimal tftp packet length is 4 bytes */ if (len < 4) @@ -276,14 +277,13 @@ 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++; - + priv->block = block + 1; tftp_timer_reset(priv); if (priv->state == STATE_LAST) { -- cgit v1.2.1 From 414945e29912b47e44a121239501f88569fa0e38 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:01 +0200 Subject: tftp: minor refactoring of RRQ/WRQ packet generation code Having 11 printf arguments with lot of them being 0, makes it difficulty to read and extend. Add some comments and use '\0' for %c. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-7-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index c1a1937117..0b2a420b66 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -134,11 +134,11 @@ static int tftp_send(struct file_priv *priv) "%d%c" "blksize%c" "1432", - priv->filename + 1, 0, - 0, - 0, - TIMEOUT, 0, - 0); + priv->filename + 1, '\0', + '\0', /* "octet" */ + '\0', /* "timeout" */ + TIMEOUT, '\0', + '\0'); /* "blksize" */ pkt++; if (!priv->push) -- cgit v1.2.1 From cc6c1edd0100d4dd72d1780acb616556a56450a0 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:02 +0200 Subject: tftp: replace hardcoded blksize by global constant Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-8-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 0b2a420b66..264841f244 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -65,6 +65,7 @@ #define STATE_DONE 8 #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */ +#define TFTP_MTU_SIZE 1432 /* MTU based block size */ #define TFTP_FIFO_SIZE 4096 #define TFTP_ERR_RESEND 1 @@ -133,12 +134,13 @@ static int tftp_send(struct file_priv *priv) "timeout%c" "%d%c" "blksize%c" - "1432", + "%u", priv->filename + 1, '\0', '\0', /* "octet" */ '\0', /* "timeout" */ TIMEOUT, '\0', - '\0'); /* "blksize" */ + '\0', /* "blksize" */ + TFTP_MTU_SIZE); pkt++; if (!priv->push) -- cgit v1.2.1 From 3fdff746b02031fbd1cf90e419d0d7694802eb9e Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:03 +0200 Subject: tftp: remove sanity check of first block With tftp window size support in the next patches, the first received block might be !=1 (e.g. when it was reordered or dropped). There could be checked whether it is in the first window, 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). Diagnostic will help only with non rfc 2743 servers (which are probably very rare resp. non existing nowadays) - the code some lines later handles this case already Remove the check and simplify things. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-9-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 8 -------- 1 file changed, 8 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 264841f244..51cb1109d2 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -321,14 +321,6 @@ static void tftp_recv(struct file_priv *priv, priv->state = STATE_RDATA; priv->tftp_con->udp->uh_dport = uh_sport; priv->last_block = 0; - - if (priv->block != 1) { /* Assertion */ - pr_err("error: First block is not block 1 (%d)\n", - priv->block); - priv->err = -EINVAL; - priv->state = STATE_DONE; - break; - } } if (priv->block == priv->last_block) -- cgit v1.2.1 From 7a52de00c64e3e04880ea6258a8b5260033f22c5 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:04 +0200 Subject: tftp: add debug_assert() macro Is a noop in normal cases (when compiler sees that the condition can be evaluated without sideeffects) but allows optimizations based on the condition. E.g. in | void foo(int a) | { | debug_assert(a == 23); | | if (a == 23) | return; | | bar(); | } the call to 'bar()' will be optimized away. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-10-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 51cb1109d2..07de8334f2 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -70,6 +70,15 @@ #define TFTP_ERR_RESEND 1 +#ifdef DEBUG +# define debug_assert(_cond) BUG_ON(!(_cond)) +#else +# define debug_assert(_cond) do { \ + if (!(_cond)) \ + __builtin_unreachable(); \ + } while (0) +#endif + struct file_priv { struct net_connection *tftp_con; int push; -- cgit v1.2.1 From 480ed057aacb5b6a3e1bdb88eaed360a295eb201 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:05 +0200 Subject: tftp: allocate buffers and fifo dynamically Use the actual blocksize for allocating buffers instead of assuming an hardcoded value. This requires to add an additional 'START' state which is entered after receiving (RRQ) or sending (WRQ) the OACK. Without it, the next state would be entered and the (not allocated yet) fifo be used. For non-rfc 2347 servers (which do not understand OACK and start with data transfer immediately after RRQ/WRQ), additional transitions in the state machine were implemented. Code adds some sanity checks in the new code paths. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-11-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 144 insertions(+), 50 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 07de8334f2..64a94797cd 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -59,14 +59,15 @@ #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 #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */ #define TFTP_MTU_SIZE 1432 /* MTU based block size */ -#define TFTP_FIFO_SIZE 4096 #define TFTP_ERR_RESEND 1 @@ -111,10 +112,10 @@ 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", + [STATE_START] = "START", }; static int tftp_send(struct file_priv *priv) @@ -166,7 +167,6 @@ static int tftp_send(struct file_priv *priv) case STATE_RDATA: if (priv->block == priv->block_requested) return 0; - case STATE_OACK: xp = pkt; s = (uint16_t *)pkt; *s++ = htons(TFTP_ACK); @@ -261,11 +261,39 @@ static void tftp_timer_reset(struct file_priv *priv) priv->progress_timeout = priv->resend_timeout = get_time_ns(); } +static int tftp_allocate_transfer(struct file_priv *priv) +{ + debug_assert(!priv->fifo); + debug_assert(!priv->buf); + + priv->fifo = kfifo_alloc(priv->blocksize); + if (!priv->fifo) + goto err; + + if (priv->push) { + priv->buf = xmalloc(priv->blocksize); + if (!priv->buf) { + kfifo_free(priv->fifo); + priv->fifo = NULL; + goto err; + } + } + + return 0; + +err: + priv->err = -ENOMEM; + priv->state = STATE_DONE; + + return priv->err; +} + static void tftp_recv(struct file_priv *priv, uint8_t *pkt, unsigned len, uint16_t uh_sport) { uint16_t opcode; uint16_t block; + int rc; /* according to RFC1350 minimal tftp packet length is 4 bytes */ if (len < 4) @@ -294,48 +322,64 @@ static void tftp_recv(struct file_priv *priv, break; } - priv->block = block + 1; - tftp_timer_reset(priv); + switch (priv->state) { + case STATE_WRQ: + priv->tftp_con->udp->uh_dport = uh_sport; + priv->state = STATE_START; + break; - if (priv->state == STATE_LAST) { + case STATE_WAITACK: + priv->state = STATE_WDATA; + break; + + 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: tftp_parse_oack(priv, pkt, len); priv->tftp_con->udp->uh_dport = uh_sport; - - if (priv->push) { - /* send first block */ - priv->state = STATE_WDATA; - priv->block = 1; - } else { - /* send ACK */ - priv->state = STATE_OACK; - priv->block = 0; - tftp_send(priv); - } - + priv->state = STATE_START; break; + case TFTP_DATA: len -= 2; priv->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; + + rc = tftp_allocate_transfer(priv); + if (rc < 0) + break; } if (priv->block == priv->last_block) /* Same block again; ignore it. */ break; + if (len > priv->blocksize) { + pr_warn("tftp: oversized packet (%u > %d) received\n", + len, priv->blocksize); + break; + } + priv->last_block = priv->block; tftp_timer_reset(priv); @@ -378,6 +422,30 @@ static void tftp_handler(void *ctx, char *packet, unsigned len) tftp_recv(priv, pkt, net_eth_to_udplen(packet), udp->uh_sport); } +static int tftp_start_transfer(struct file_priv *priv) +{ + int rc; + + rc = tftp_allocate_transfer(priv); + if (rc < 0) + /* function sets 'priv->state = STATE_DONE' and 'priv->err' in + error case */ + return rc; + + if (priv->push) { + /* send first block */ + priv->state = STATE_WDATA; + priv->block = 1; + } else { + /* send ACK */ + priv->state = STATE_RDATA; + priv->block = 0; + tftp_send(priv); + } + + return 0; +} + static struct file_priv *tftp_do_open(struct device_d *dev, int accmode, struct dentry *dentry) { @@ -409,48 +477,74 @@ static struct file_priv *tftp_do_open(struct device_d *dev, priv->blocksize = TFTP_BLOCK_SIZE; priv->block_requested = -1; - priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE); - if (!priv->fifo) { - ret = -ENOMEM; - goto out; - } - parseopt_hu(fsdev->options, "port", &port); priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv); if (IS_ERR(priv->tftp_con)) { ret = PTR_ERR(priv->tftp_con); - goto out1; + goto out; } ret = tftp_send(priv); if (ret) - goto out2; + goto out1; tftp_timer_reset(priv); - while (priv->state != STATE_RDATA && - priv->state != STATE_DONE && - priv->state != STATE_WDATA) { - ret = tftp_poll(priv); - if (ret == TFTP_ERR_RESEND) - tftp_send(priv); - if (ret < 0) - goto out2; - } - if (priv->state == STATE_DONE && priv->err) { - ret = priv->err; - goto out2; - } + /* - '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; - priv->buf = xmalloc(priv->blocksize); + 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); + + if (ret < 0) + goto out1; return priv; -out2: - net_unregister(priv->tftp_con); out1: - kfifo_free(priv->fifo); + net_unregister(priv->tftp_con); out: + if (priv->fifo) + kfifo_free(priv->fifo); + + free(priv->filename); + free(priv->buf); free(priv); return ERR_PTR(ret); @@ -567,7 +661,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize) if (priv->state == STATE_DONE) return outsize; - if (TFTP_FIFO_SIZE - kfifo_len(priv->fifo) >= priv->blocksize) + if (kfifo_len(priv->fifo) == 0) tftp_send(priv); ret = tftp_poll(priv); -- cgit v1.2.1 From 56c5c40a52b7541f2b1379a1645bab2a33a8a83e Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:06 +0200 Subject: tftp: add sanity check for OACK response Catch bad 'blocksize' or 'windowsize' responses from the server. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-12-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 64a94797cd..4e31adcd60 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -229,7 +229,7 @@ static int tftp_poll(struct file_priv *priv) return 0; } -static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len) +static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len) { unsigned char *opt, *val, *s; @@ -246,7 +246,7 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len) opt = s; val = s + strlen(s) + 1; if (val > s + len) - return; + break; if (!strcmp(opt, "tsize")) priv->filesize = simple_strtoull(val, NULL, 10); if (!strcmp(opt, "blksize")) @@ -254,6 +254,13 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len) pr_debug("OACK opt: %s val: %s\n", opt, val); s = val + strlen(val) + 1; } + + if (priv->blocksize > TFTP_MTU_SIZE) { + pr_warn("tftp: invalid oack response\n"); + return -EINVAL; + } + + return 0; } static void tftp_timer_reset(struct file_priv *priv) @@ -349,8 +356,14 @@ static void tftp_recv(struct file_priv *priv, break; case TFTP_OACK: - tftp_parse_oack(priv, pkt, len); priv->tftp_con->udp->uh_dport = uh_sport; + + if (tftp_parse_oack(priv, pkt, len) < 0) { + priv->err = -EINVAL; + priv->state = STATE_DONE; + break; + } + priv->state = STATE_START; break; -- cgit v1.2.1 From 503b04c4966bcba88b7a1c3babb8584c1101170b Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:07 +0200 Subject: tftp: record whether tftp file is opened for lookup operation only Opening a tftp is done in two steps: at first `tftp_lookup()` is called to get the filesize and then it is opened again and data are read. The `tftp_lookup()` call sends only a RRQ/WRQ, reads then the "tsize" from the response and closes the transfer by sending an error datagram. The tftp server will send a full data window. To prevent unneeded traffic, later patches set parameters to reduce the size of the server response. We need knowledge about type of operation which is recorded in an "is_getattr" attribute. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-13-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 4e31adcd60..7181c58d10 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -95,6 +95,7 @@ struct file_priv { void *buf; int blocksize; int block_requested; + bool is_getattr; }; struct tftp_priv { @@ -460,7 +461,7 @@ static int tftp_start_transfer(struct file_priv *priv) } static struct file_priv *tftp_do_open(struct device_d *dev, - int accmode, struct dentry *dentry) + int accmode, struct dentry *dentry, bool is_getattr) { struct fs_device_d *fsdev = dev_to_fs_device(dev); struct file_priv *priv; @@ -489,6 +490,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev, priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root); priv->blocksize = TFTP_BLOCK_SIZE; priv->block_requested = -1; + priv->is_getattr = is_getattr; parseopt_hu(fsdev->options, "port", &port); @@ -567,7 +569,7 @@ static int tftp_open(struct device_d *dev, FILE *file, const char *filename) { struct file_priv *priv; - priv = tftp_do_open(dev, file->flags, file->dentry); + priv = tftp_do_open(dev, file->flags, file->dentry, false); if (IS_ERR(priv)) return PTR_ERR(priv); @@ -783,7 +785,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry, struct file_priv *priv; loff_t filesize; - priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry); + priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry, true); if (IS_ERR(priv)) return NULL; -- cgit v1.2.1 From 9821f7f47a1fd2b5e250c2b4ed39a5364a00eb07 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:08 +0200 Subject: tftp: reduce block size on lookup requests Save some bytes on network traffic by reducing the server response for lookup requests. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-14-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 7181c58d10..16e57daaef 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -151,7 +151,9 @@ static int tftp_send(struct file_priv *priv) '\0', /* "timeout" */ TIMEOUT, '\0', '\0', /* "blksize" */ - TFTP_MTU_SIZE); + /* use only a minimal blksize for getattr + operations, */ + priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE); pkt++; if (!priv->push) -- cgit v1.2.1 From 9e21f4a6184f020dbc89fd0613f57e8519365454 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:09 +0200 Subject: tftp: refactor data processing move block handling into dedicated function Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-15-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 16e57daaef..5b0dcb2b75 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -298,6 +298,26 @@ err: return priv->err; } +static void tftp_put_data(struct file_priv *priv, uint16_t block, + void const *pkt, size_t len) +{ + if (len > priv->blocksize) { + pr_warn("tftp: oversized packet (%zu > %d) received\n", + len, priv->blocksize); + return; + } + + priv->last_block = block; + + kfifo_put(priv->fifo, pkt, len); + + if (len < priv->blocksize) { + tftp_send(priv); + priv->err = 0; + priv->state = STATE_DONE; + } +} + static void tftp_recv(struct file_priv *priv, uint8_t *pkt, unsigned len, uint16_t uh_sport) { @@ -390,23 +410,8 @@ static void tftp_recv(struct file_priv *priv, /* Same block again; ignore it. */ break; - if (len > priv->blocksize) { - pr_warn("tftp: oversized packet (%u > %d) received\n", - len, priv->blocksize); - break; - } - - priv->last_block = priv->block; - tftp_timer_reset(priv); - - kfifo_put(priv->fifo, pkt + 2, len); - - if (len < priv->blocksize) { - tftp_send(priv); - priv->err = 0; - priv->state = STATE_DONE; - } + tftp_put_data(priv, priv->block, pkt + 2, len); break; -- cgit v1.2.1 From b878b167cba028d7ab264d100a6c2c4bf3a460f3 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:10 +0200 Subject: tftp: detect out-of-memory situations it should never happen due to the program logic; but detect a failed kfifo_put() just in case... Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-16-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 5b0dcb2b75..cabe923329 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -301,6 +301,8 @@ err: static void tftp_put_data(struct file_priv *priv, uint16_t block, void const *pkt, size_t len) { + unsigned int sz; + if (len > priv->blocksize) { pr_warn("tftp: oversized packet (%zu > %d) received\n", len, priv->blocksize); @@ -309,9 +311,14 @@ static void tftp_put_data(struct file_priv *priv, uint16_t block, priv->last_block = block; - kfifo_put(priv->fifo, pkt, len); + sz = kfifo_put(priv->fifo, pkt, len); - if (len < priv->blocksize) { + if (sz != len) { + pr_err("tftp: not enough room in kfifo (only %u out of %zu written)\n", + sz, len); + priv->err = -ENOMEM; + priv->state = STATE_DONE; + } else if (len < priv->blocksize) { tftp_send(priv); priv->err = 0; priv->state = STATE_DONE; -- cgit v1.2.1 From 320ce55adcb6bc2c426ec98bd7e97bcbd6873ce1 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:11 +0200 Subject: tftp: implement 'windowsize' (RFC 7440) support Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are: | windowsize | VPN | 1 Gb/s | 100 Mb/s | |------------|-----------|------------|------------| | 128 | 3.869.284 | 98.643.085 | 11.434.852 | | 64 | 3.863.581 | 98.550.375 | 11.434.852 | | 48 | 3.431.580 | 94.211.680 | 11.275.010 | | 32 | 2.835.129 | 85.250.081 | 10.985.605 | | 24 | 2.344.858 | 77.787.537 | 10.765.667 | | 16 | 1.734.186 | 67.519.381 | 10.210.087 | | 12 | 1.403.340 | 61.972.576 | 9.915.612 | | 8 | 1.002.462 | 50.852.376 | 9.016.130 | | 6 | 775.573 | 42.781.558 | 8.422.297 | | 4 | 547.845 | 32.066.544 | 6.835.567 | | 3 | 412.987 | 26.526.081 | 6.322.435 | | 2 | 280.987 | 19.120.641 | 5.494.241 | | 1 | 141.699 | 10.431.516 | 2.967.224 | (VPN = OpenVPN on ADSL 50 Mb/s). The window size can be configured at runtime. This commit increases barebox size by | add/remove: 1/0 grow/shrink: 4/1 up/down: 148/-16 (132) | Function old new delta | tftp_send 336 384 +48 | tftp_handler 880 928 +48 | tftp_init 16 60 +44 | tftp_allocate_transfer 100 104 +4 | g_tftp_window_size - 4 +4 | tftp_poll 180 164 -16 | Total: Before=629980, After=630112, chg +0.02% Setting FS_TFTP_MAX_WINDOW_SIZE to zero reduces it to | add/remove: 1/0 grow/shrink: 3/2 up/down: 96/-52 (44) | Function old new delta | tftp_init 16 60 +44 | tftp_handler 880 924 +44 | tftp_allocate_transfer 100 104 +4 | g_tftp_window_size - 4 +4 | tftp_poll 180 164 -16 | tftp_send 336 300 -36 | Total: Before=629980, After=630024, chg +0.01% Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-17-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/Kconfig | 14 ++++++++++++++ fs/tftp.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 64 insertions(+), 13 deletions(-) (limited to 'fs') diff --git a/fs/Kconfig b/fs/Kconfig index aeba00073e..0c49342859 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -43,6 +43,20 @@ config FS_TFTP prompt "tftp support" depends on NET +config FS_TFTP_MAX_WINDOW_SIZE + int + prompt "maximum tftp window size (RFC 7440)" + depends on FS_TFTP + default 128 + range 1 128 + help + The maximum allowed tftp "windowsize" (RFC 7440). Higher + value increase speed of the tftp download with the cost of + memory (1432 bytes per slot). + + Requires tftp "windowsize" (RFC 7440) support on server side + to have an effect. + config FS_OMAP4_USBBOOT bool prompt "Filesystem over usb boot" diff --git a/fs/tftp.c b/fs/tftp.c index cabe923329..98ac34bf5a 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -26,9 +26,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -68,6 +70,10 @@ #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */ #define TFTP_MTU_SIZE 1432 /* MTU based block size */ +#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE + +/* allocate this number of blocks more than needed in the fifo */ +#define TFTP_EXTRA_BLOCKS 2 #define TFTP_ERR_RESEND 1 @@ -80,11 +86,14 @@ } while (0) #endif +static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2); + struct file_priv { struct net_connection *tftp_con; int push; uint16_t block; uint16_t last_block; + uint16_t ack_block; int state; int err; char *filename; @@ -94,7 +103,7 @@ struct file_priv { struct kfifo *fifo; void *buf; int blocksize; - int block_requested; + unsigned int windowsize; bool is_getattr; }; @@ -125,6 +134,7 @@ static int tftp_send(struct file_priv *priv) int len = 0; uint16_t *s; unsigned char *pkt = net_udp_get_payload(priv->tftp_con); + unsigned int window_size; int ret; pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]); @@ -132,6 +142,15 @@ static int tftp_send(struct file_priv *priv) switch (priv->state) { case STATE_RRQ: case STATE_WRQ: + if (priv->push || priv->is_getattr) + /* atm, windowsize is supported only for RRQ and there + is no need to request a full window when we are + just looking up file attributes */ + window_size = 1; + else + window_size = min_t(unsigned int, g_tftp_window_size, + TFTP_MAX_WINDOW_SIZE); + xp = pkt; s = (uint16_t *)pkt; if (priv->state == STATE_RRQ) @@ -164,17 +183,22 @@ static int tftp_send(struct file_priv *priv) '\0', priv->filesize, '\0'); + if (window_size > 1) + pkt += sprintf((unsigned char *)pkt, + "windowsize%c%u%c", + '\0', window_size, + '\0'); + len = pkt - xp; break; case STATE_RDATA: - if (priv->block == priv->block_requested) - return 0; xp = pkt; s = (uint16_t *)pkt; *s++ = htons(TFTP_ACK); - *s++ = htons(priv->block); - priv->block_requested = priv->block; + *s++ = htons(priv->last_block); + priv->ack_block = priv->last_block; + priv->ack_block += priv->windowsize; pkt = (unsigned char *)s; len = pkt - xp; break; @@ -217,7 +241,6 @@ static int tftp_poll(struct file_priv *priv) if (is_timeout(priv->resend_timeout, TFTP_RESEND_TIMEOUT)) { printf("T "); priv->resend_timeout = get_time_ns(); - priv->block_requested = -1; return TFTP_ERR_RESEND; } @@ -254,11 +277,15 @@ static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len) priv->filesize = simple_strtoull(val, NULL, 10); if (!strcmp(opt, "blksize")) priv->blocksize = simple_strtoul(val, NULL, 10); + if (!strcmp(opt, "windowsize")) + priv->windowsize = simple_strtoul(val, NULL, 10); pr_debug("OACK opt: %s val: %s\n", opt, val); s = val + strlen(val) + 1; } - if (priv->blocksize > TFTP_MTU_SIZE) { + if (priv->blocksize > TFTP_MTU_SIZE || + priv->windowsize > TFTP_MAX_WINDOW_SIZE || + priv->windowsize == 0) { pr_warn("tftp: invalid oack response\n"); return -EINVAL; } @@ -276,7 +303,10 @@ static int tftp_allocate_transfer(struct file_priv *priv) debug_assert(!priv->fifo); debug_assert(!priv->buf); - priv->fifo = kfifo_alloc(priv->blocksize); + /* multiplication is safe; both operands were checked in tftp_parse_oack() + and are small integers */ + priv->fifo = kfifo_alloc(priv->blocksize * + (priv->windowsize + TFTP_EXTRA_BLOCKS)); if (!priv->fifo) goto err; @@ -407,14 +437,14 @@ static void tftp_recv(struct file_priv *priv, priv->tftp_con->udp->uh_dport = uh_sport; priv->state = STATE_RDATA; priv->last_block = 0; + priv->ack_block = priv->windowsize; rc = tftp_allocate_transfer(priv); if (rc < 0) break; } - if (priv->block == priv->last_block) - /* Same block again; ignore it. */ + if (priv->block != (uint16_t)(priv->last_block + 1)) break; tftp_timer_reset(priv); @@ -467,7 +497,7 @@ static int tftp_start_transfer(struct file_priv *priv) } else { /* send ACK */ priv->state = STATE_RDATA; - priv->block = 0; + priv->last_block = 0; tftp_send(priv); } @@ -503,7 +533,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev, priv->err = -EINVAL; priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root); priv->blocksize = TFTP_BLOCK_SIZE; - priv->block_requested = -1; + priv->windowsize = 1; priv->is_getattr = is_getattr; parseopt_hu(fsdev->options, "port", &port); @@ -690,7 +720,12 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize) if (priv->state == STATE_DONE) return outsize; - if (kfifo_len(priv->fifo) == 0) + /* send the ACK only when fifo has been nearly depleted; else, + when tftp_read() is called with small 'insize' values, it + is possible that there is read more data from the network + than consumed by kfifo_get() and the fifo overflows */ + if (priv->last_block == priv->ack_block && + kfifo_len(priv->fifo) <= TFTP_EXTRA_BLOCKS * priv->blocksize) tftp_send(priv); ret = tftp_poll(priv); @@ -882,6 +917,8 @@ static struct fs_driver_d tftp_driver = { static int tftp_init(void) { + globalvar_add_simple_int("tftp.windowsize", &g_tftp_window_size, "%u"); + return register_fs_driver(&tftp_driver); } coredevice_initcall(tftp_init); -- cgit v1.2.1 From 46ff23b4468dabf5cec3ca6db1bad8dab4818943 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:12 +0200 Subject: tftp: do not use 'priv->block' for RRQ attribute is not used outside tftp_recv() for RRQ. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-18-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 98ac34bf5a..c2f77e3d87 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -429,7 +429,7 @@ static void tftp_recv(struct file_priv *priv, case TFTP_DATA: len -= 2; - priv->block = ntohs(*(uint16_t *)pkt); + block = ntohs(*(uint16_t *)pkt); if (priv->state == STATE_RRQ) { /* first block received; entered only with non rfc @@ -444,11 +444,11 @@ static void tftp_recv(struct file_priv *priv, break; } - if (priv->block != (uint16_t)(priv->last_block + 1)) + if (block != (uint16_t)(priv->last_block + 1)) break; tftp_timer_reset(priv); - tftp_put_data(priv, priv->block, pkt + 2, len); + tftp_put_data(priv, block, pkt + 2, len); break; -- cgit v1.2.1 From 207210e9d07f3437b67e6a6fb15843052ebe3ac8 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:13 +0200 Subject: tftp: reorder tftp packets With the tftp "windowsize" option, reordering of udp datagrams becomes an issue. Depending on the network topology, this reordering occurs several times with large tftp transfers and will heavily reduce the transfer speed. This patch adds a packet cache so that datagrams can be reassembled in the correct order. Because it increases memory usage and barebox binary size, it is an Kconfig option. bloat-o-meter reports with a non-zero FS_TFTP_REORDER_CACHE_SIZE | add/remove: 3/0 grow/shrink: 4/0 up/down: 916/0 (916) | Function old new delta | tftp_handler 920 1244 +324 | tftp_put_data - 184 +184 | tftp_window_cache_remove - 124 +124 | tftp_window_cache_get_pos - 120 +120 | tftp_allocate_transfer 104 188 +84 | tftp_do_close 260 312 +52 | tftp_send 384 412 +28 | Total: Before=630104, After=631020, chg +0.15% After setting FS_TFTP_REORDER_CACHE_SIZE Kconfig option to 0, numbers are going down to | add/remove: 0/0 grow/shrink: 3/0 up/down: 152/0 (152) | Function old new delta | tftp_handler 920 1012 +92 | tftp_allocate_transfer 104 136 +32 | tftp_send 384 412 +28 | Total: Before=630104, After=630256, chg +0.02% Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-19-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/Kconfig | 22 +++++ fs/tftp.c | 307 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 323 insertions(+), 6 deletions(-) (limited to 'fs') diff --git a/fs/Kconfig b/fs/Kconfig index 0c49342859..cf884e0646 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -57,6 +57,28 @@ config FS_TFTP_MAX_WINDOW_SIZE Requires tftp "windowsize" (RFC 7440) support on server side to have an effect. +config FS_TFTP_REORDER_CACHE_SIZE + int + prompt "number of out-of-order tftp packets to be cached" + depends on FS_TFTP + default 16 if FS_TFTP_MAX_WINDOW_SIZE > 16 + default 0 if FS_TFTP_MAX_WINDOW_SIZE = 1 + ## TODO: it should be 'FS_TFTP_MAX_WINDOW_SIZE - 1' but this + ## is not supported by Kconfig + default FS_TFTP_MAX_WINDOW_SIZE + range 0 FS_TFTP_MAX_WINDOW_SIZE + help + UDP allows reordering of datagrams; with this option, + unexpected tftp packets will be cached and later + reassembled. This increases stability of the tftp download + with the cost of memory (around 1440 bytes per cache + element) and barebox binary size (around 700 bytes). + + A value of 0 disables caching. + + Requires tftp "windowsize" (RFC 7440) support on server side + to have an effect. + config FS_OMAP4_USBBOOT bool prompt "Filesystem over usb boot" diff --git a/fs/tftp.c b/fs/tftp.c index c2f77e3d87..42fe121c68 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -75,6 +76,12 @@ /* allocate this number of blocks more than needed in the fifo */ #define TFTP_EXTRA_BLOCKS 2 +/* size of cache which deals with udp reordering */ +#define TFTP_WINDOW_CACHE_NUM CONFIG_FS_TFTP_REORDER_CACHE_SIZE + +/* marker for an emtpy 'tftp_cache' */ +#define TFTP_CACHE_NO_ID (-1) + #define TFTP_ERR_RESEND 1 #ifdef DEBUG @@ -88,6 +95,30 @@ static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2); +struct tftp_block { + uint16_t id; + uint16_t len; + + struct list_head head; + uint8_t data[]; +}; + +struct tftp_cache { + /* The id located at @pos or TFTP_CACHE_NO_ID when cache is empty. It + is possible that the corresponding bit in @used is NOT set. */ + unsigned int id; + unsigned int pos; + + /* bitmask */ + unsigned long used[BITS_TO_LONGS(TFTP_WINDOW_CACHE_NUM)]; + + /* size of the cache; is limited by TFTP_WINDOW_CACHE_NUM and the + actual window size */ + unsigned int size; + unsigned int block_len; + struct tftp_block *blocks[TFTP_WINDOW_CACHE_NUM]; +}; + struct file_priv { struct net_connection *tftp_con; int push; @@ -105,12 +136,222 @@ struct file_priv { int blocksize; unsigned int windowsize; bool is_getattr; + struct tftp_cache cache; }; struct tftp_priv { IPaddr_t server; }; +static inline bool is_block_before(uint16_t a, uint16_t b) +{ + return (int16_t)(b - a) > 0; +} + +static inline uint16_t get_block_delta(uint16_t a, uint16_t b) +{ + debug_assert(!is_block_before(b, a)); + + return b - a; +} + +static bool in_window(uint16_t block, uint16_t start, uint16_t end) +{ + /* handle the three cases: + - [ ......... | start | .. | BLOCK | .. | end | ......... ] + - [ ..| BLOCK | .. | end | ................. | start | .. ] + - [ ..| end | ................. | start | .. | BLOCK | .. ] + */ + return ((start <= block && block <= end) || + (block <= end && end <= start) || + (end <= start && start <= block)); +} + +static inline size_t tftp_window_cache_size(struct tftp_cache const *cache) +{ + /* allows to optimize away the cache code when TFTP_WINDOW_CACHE_SIZE + is 0 */ + return TFTP_WINDOW_CACHE_NUM == 0 ? 0 : cache->size; +} + +static void tftp_window_cache_init(struct tftp_cache *cache, + uint16_t block_len, uint16_t window_size) +{ + debug_assert(window_size > 0); + + *cache = (struct tftp_cache) { + .id = TFTP_CACHE_NO_ID, + .block_len = block_len, + .size = min_t(uint16_t, window_size - 1, + ARRAY_SIZE(cache->blocks)), + }; +} + +static void tftp_window_cache_free(struct tftp_cache *cache) +{ + size_t cache_size = tftp_window_cache_size(cache); + size_t i; + + for (i = 0; i < cache_size; ++i) { + free(cache->blocks[i]); + cache->blocks[i] = NULL; + } +} + +static void tftp_window_cache_reset(struct tftp_cache *cache) +{ + cache->id = TFTP_CACHE_NO_ID; + memset(cache->used, 0, sizeof cache->used); +} + +static int tftp_window_cache_get_pos(struct tftp_cache const *cache, uint16_t id) +{ + size_t cache_size = tftp_window_cache_size(cache); + unsigned int pos; + + if (cache_size == 0) + return -1; + + if (cache->id == TFTP_CACHE_NO_ID) + return -1; + + if (!in_window(id, cache->id, cache->id + cache_size - 1)) + return -1; + + pos = cache->pos + get_block_delta(cache->id, id); + pos %= cache_size; + + return pos; +} + +/* returns whether the first cached element has the given @id */ +static bool tftp_window_cache_starts_with(struct tftp_cache const *cache, + uint16_t id) +{ + return (TFTP_WINDOW_CACHE_NUM > 0 && + cache->id != TFTP_CACHE_NO_ID && + cache->id == id && + test_bit(cache->pos, cache->used)); +} + +static bool tftp_window_cache_is_empty(struct tftp_cache const *cache) +{ + /* use this indirection to avoid warnings about a '0 < 0' comparison + in the loop condition when TFTP_WINDOW_CACHE_NUM is zero */ + size_t cache_size = ARRAY_SIZE(cache->used); + size_t i; + + for (i = 0; i < cache_size; ++i) { + if (cache->used[i] != 0) + return false; + } + + return true; +} + +static int tftp_window_cache_insert(struct tftp_cache *cache, uint16_t id, + void const *data, size_t len) +{ + size_t const cache_size = tftp_window_cache_size(cache); + int pos; + struct tftp_block *block; + + if (cache_size == 0) + return -ENOSPC; + + if (cache->id == TFTP_CACHE_NO_ID) { + /* initialize cache when it does not contain elements yet */ + cache->id = id; + cache->pos = 0; + + /* sanity check; cache is expected to be empty */ + debug_assert(tftp_window_cache_is_empty(cache)); + } + + pos = tftp_window_cache_get_pos(cache, id); + if (pos < 0) + return -ENOSPC; + + debug_assert(pos < cache_size); + + if (test_bit(pos, cache->used)) + /* block already cached */ + return 0; + + block = cache->blocks[pos]; + if (!block) { + /* allocate space for the block; after being released, this + memory can be reused for other blocks during the same tftp + transfer */ + block = malloc(sizeof *block + cache->block_len); + if (!block) + return -ENOMEM; + + cache->blocks[pos] = block; + } + + __set_bit(pos, cache->used); + memcpy(block->data, data, len); + block->id = id; + block->len = len; + + return 0; +} + +/* Marks the element at 'pos' as unused and updates internal cache information. + Returns true iff element at pos was valid. */ +static bool tftp_window_cache_remove(struct tftp_cache *cache, unsigned int pos) +{ + size_t const cache_size = tftp_window_cache_size(cache); + bool res; + + if (cache_size == 0) + return 0; + + res = __test_and_clear_bit(pos, cache->used); + + if (tftp_window_cache_is_empty(cache)) { + /* cache has been cleared; reset pointers */ + cache->id = TFTP_CACHE_NO_ID; + } else if (pos != cache->pos) { + /* noop; elements has been removed from the middle of cache */ + } else { + /* first element of cache has been removed; proceed to the + next one */ + cache->id += 1; + cache->pos += 1; + cache->pos %= cache_size; + } + + return res; +} + +/* Releases the first element from the cache and returns its content. + * + * Function can return NULL when the element is not cached + */ +static struct tftp_block *tftp_window_cache_pop(struct tftp_cache *cache) +{ + unsigned int pos = cache->pos; + + debug_assert(cache->id != TFTP_CACHE_NO_ID); + + if (!tftp_window_cache_remove(cache, pos)) + return NULL; + + return cache->blocks[pos]; +} + +static bool tftp_window_cache_remove_id(struct tftp_cache *cache, uint16_t id) +{ + int pos = tftp_window_cache_get_pos(cache, id); + + if (pos < 0) + return false; + + return tftp_window_cache_remove(cache, pos); +} + static int tftp_truncate(struct device_d *dev, FILE *f, loff_t size) { return 0; @@ -201,6 +442,7 @@ static int tftp_send(struct file_priv *priv) priv->ack_block += priv->windowsize; pkt = (unsigned char *)s; len = pkt - xp; + tftp_window_cache_reset(&priv->cache); break; } @@ -317,6 +559,9 @@ static int tftp_allocate_transfer(struct file_priv *priv) priv->fifo = NULL; goto err; } + } else { + tftp_window_cache_init(&priv->cache, + priv->blocksize, priv->windowsize); } return 0; @@ -355,6 +600,60 @@ static void tftp_put_data(struct file_priv *priv, uint16_t block, } } +static void tftp_apply_window_cache(struct file_priv *priv) +{ + struct tftp_cache *cache = &priv->cache; + + while (tftp_window_cache_starts_with(cache, priv->last_block + 1)) { + struct tftp_block *block; + + /* can be changed by tftp_put_data() below and must be + checked in each loop */ + if (priv->state != STATE_RDATA) + return; + + block = tftp_window_cache_pop(cache); + + debug_assert(block); + debug_assert(block->id == (uint16_t)(priv->last_block + 1)); + + tftp_put_data(priv, block->id, block->data, block->len); + } +} + +static void tftp_handle_data(struct file_priv *priv, uint16_t block, + void const *data, size_t len) +{ + uint16_t exp_block; + int rc; + + exp_block = priv->last_block + 1; + + if (exp_block == block) { + /* datagram over network is the expected one; put it in the + fifo directly and try to apply cached items then */ + tftp_timer_reset(priv); + tftp_put_data(priv, block, data, len); + tftp_window_cache_remove_id(&priv->cache, block); + tftp_apply_window_cache(priv); + } else if (!in_window(block, exp_block, priv->ack_block)) { + /* completely unexpected and unrelated to actual window; + ignore the packet. */ + printf("B"); + } else { + /* The 'rc < 0' below happens e.g. when datagrams in the first + part of the transfer window are dropped. + + TODO: this will usually result in a timeout + (TFTP_RESEND_TIMEOUT). It should be possible to bypass + this timeout by acknowledging the last packet (e.g. by + doing 'priv->ack_block = priv->last_block' here). */ + rc = tftp_window_cache_insert(&priv->cache, block, data, len); + if (rc < 0) + printf("M"); + } +} + static void tftp_recv(struct file_priv *priv, uint8_t *pkt, unsigned len, uint16_t uh_sport) { @@ -444,12 +743,7 @@ static void tftp_recv(struct file_priv *priv, break; } - if (block != (uint16_t)(priv->last_block + 1)) - break; - - tftp_timer_reset(priv); - tftp_put_data(priv, block, pkt + 2, len); - + tftp_handle_data(priv, block, pkt + 2, len); break; case TFTP_ERROR: @@ -653,6 +947,7 @@ static int tftp_do_close(struct file_priv *priv) } net_unregister(priv->tftp_con); + tftp_window_cache_free(&priv->cache); kfifo_free(priv->fifo); free(priv->filename); free(priv->buf); -- cgit v1.2.1 From 32867e6d6e49cc02057476cd4fbeb726ca1ee9e4 Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:14 +0200 Subject: tftp: add selftest Unittest for window cache functions. Signed-off-by: Enrico Scholz Link: https://lore.barebox.org/20220830073816.2694734-20-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp-selftest.h | 56 ++++++++++++++++++++++++++++ fs/tftp.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 fs/tftp-selftest.h (limited to 'fs') diff --git a/fs/tftp-selftest.h b/fs/tftp-selftest.h new file mode 100644 index 0000000000..2406ed329e --- /dev/null +++ b/fs/tftp-selftest.h @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: GPL-2.0+ +// SPDX-FileCopyrightText: 2022 Enrico Scholz + +#ifndef H_BAREBOX_FS_TFTP_SELFTEST_H +#define H_BAREBOX_FS_TFTP_SELFTEST_H + +#include + +#define _expect_fmt(_v) _Generic(_v, \ + void const *: "%p", \ + void *: "%p", \ + bool: "%d", \ + long int: "%lld", \ + long unsigned int: "%llu", \ + unsigned short: "%h", \ + signed int: "%d", \ + unsigned int: "%u") + +#define _expect_op(_a, _b, _op) \ + ({ \ + __typeof__(_a) __a = (_a); \ + __typeof__(_b) __b = (_b); \ + \ + total_tests++; \ + \ + if (!(__a _op __b)) { \ + char fmt[sizeof "%s:%d: failed: %XXX %XXX\n" # _op]; \ + strcpy(fmt, "%s:%d: failed: "); \ + strcat(fmt, _expect_fmt(__a)); \ + strcat(fmt, " " # _op " "); \ + strcat(fmt, _expect_fmt(__b)); \ + strcat(fmt, "\n"); \ + \ + failed_tests++; \ + printf(fmt, __func__, __LINE__, __a, __b); \ + } \ + }) + +#define _as_void(_p) ({ \ + void const *__res = (_p); \ + __res; \ + }) + +#define expect_eq(_a, _b) _expect_op(_a, _b, ==); +#define expect_ne(_a, _b) _expect_op(_a, _b, !=); +#define expect_it(_a) _expect_op(_a, true, ==); + +#define expect_err(_a) _expect_op(_a, 0, <); +#define expect_ok(_a) _expect_op(_a, 0, ==); + +/* _Generic() does not match 'void *' for typed pointers; cast them to raw + 'void *' first */ +#define expect_NULL(_a) _expect_op(_as_void(_a), NULL, ==); +#define expect_PTR(_a) _expect_op(_as_void(_a), NULL, !=); + +#endif /* H_BAREBOX_FS_TFTP_SELFTEST_H */ diff --git a/fs/tftp.c b/fs/tftp.c index 42fe121c68..37180b8675 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -36,6 +36,8 @@ #include #include +#include "tftp-selftest.h" + #define TFTP_PORT 69 /* Well known TFTP port number */ /* Seconds to wait before remote server is allowed to resend a lost packet */ @@ -84,7 +86,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 { \ @@ -1217,3 +1219,105 @@ static int tftp_init(void) return register_fs_driver(&tftp_driver); } coredevice_initcall(tftp_init); + + +BSELFTEST_GLOBALS(); + +static int __maybe_unused tftp_window_cache_selftest(void) +{ + struct tftp_cache *cache = malloc(sizeof *cache); + + if (!cache) + return -ENOMEM; + + (void)skipped_tests; + + expect_it( is_block_before(0, 1)); + expect_it(!is_block_before(1, 0)); + expect_it( is_block_before(65535, 0)); + expect_it(!is_block_before(0, 65535)); + + expect_eq(get_block_delta(0, 1), 1); + expect_eq(get_block_delta(65535, 0), 1); + expect_eq(get_block_delta(65535, 1), 2); + + expect_it(!in_window(0, 1, 3)); + expect_it( in_window(1, 1, 3)); + expect_it( in_window(2, 1, 3)); + expect_it( in_window(3, 1, 3)); + expect_it(!in_window(4, 1, 3)); + + expect_it(!in_window(65534, 65535, 1)); + expect_it( in_window(65535, 65535, 1)); + expect_it( in_window( 0, 65535, 1)); + expect_it( in_window( 1, 65535, 1)); + expect_it(!in_window( 2, 65535, 1)); + + + tftp_window_cache_init(cache, 512, 5); + + if (tftp_window_cache_size(cache) < 4) + goto out; + + expect_eq(tftp_window_cache_size(cache), 4); + + /* sequence 1 */ + expect_ok (tftp_window_cache_insert(cache, 20, "20", 2)); + expect_ok (tftp_window_cache_insert(cache, 22, "22", 2)); + expect_ok (tftp_window_cache_insert(cache, 21, "21", 2)); + expect_ok (tftp_window_cache_insert(cache, 23, "23", 2)); + expect_err(tftp_window_cache_insert(cache, 24, "24", 2)); + expect_err(tftp_window_cache_insert(cache, 19, "19", 2)); + expect_ok (tftp_window_cache_insert(cache, 22, "22", 2)); + expect_ok (tftp_window_cache_insert(cache, 20, "20", 2)); + + expect_eq(tftp_window_cache_pop(cache)->id, 20); + expect_eq(tftp_window_cache_pop(cache)->id, 21); + expect_eq(tftp_window_cache_pop(cache)->id, 22); + expect_eq(tftp_window_cache_pop(cache)->id, 23); + expect_eq(cache->id, TFTP_CACHE_NO_ID); + + /* sequence 2 */ + expect_ok (tftp_window_cache_insert(cache, 30, "30", 2)); + expect_ok (tftp_window_cache_insert(cache, 32, "32", 2)); + expect_err(tftp_window_cache_insert(cache, 34, "34", 2)); + + expect_it(tftp_window_cache_starts_with(cache, 30)); + expect_eq(tftp_window_cache_pop(cache)->id, 30); + + expect_ok (tftp_window_cache_insert(cache, 34, "34", 2)); + expect_err(tftp_window_cache_insert(cache, 35, "35", 2)); + + expect_it(!tftp_window_cache_starts_with(cache, 30)); + expect_it(!tftp_window_cache_starts_with(cache, 31)); + expect_it(!tftp_window_cache_starts_with(cache, 32)); + expect_NULL(tftp_window_cache_pop(cache)); + + expect_it(tftp_window_cache_starts_with(cache, 32)); + expect_eq(tftp_window_cache_pop(cache)->id, 32); + + expect_NULL(tftp_window_cache_pop(cache)); + expect_eq(tftp_window_cache_pop(cache)->id, 34); + + expect_eq(cache->id, TFTP_CACHE_NO_ID); + + /* sequence 3 */ + expect_ok(tftp_window_cache_insert(cache, 40, "40", 2)); + expect_ok(tftp_window_cache_insert(cache, 42, "42", 2)); + expect_ok(tftp_window_cache_insert(cache, 43, "43", 2)); + + expect_it(!tftp_window_cache_remove_id(cache, 30)); + expect_it(!tftp_window_cache_remove_id(cache, 41)); + expect_it(!tftp_window_cache_remove_id(cache, 44)); + + expect_it( tftp_window_cache_remove_id(cache, 42)); + expect_it(!tftp_window_cache_remove_id(cache, 42)); + +out: + tftp_window_cache_free(cache); + + return 0; +} +#ifdef CONFIG_SELFTEST_TFTP +bselftest(core, tftp_window_cache_selftest); +#endif -- cgit v1.2.1 From a21b7ddc5a90868eda665c69004c43b37beea6ca Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Tue, 30 Aug 2022 09:38:15 +0200 Subject: tftp: accept OACK + DATA datagrams only in certain states 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 Link: https://lore.barebox.org/20220830073816.2694734-21-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index 37180b8675..ada6ad08de 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -717,6 +717,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) { @@ -745,6 +751,12 @@ static void tftp_recv(struct file_priv *priv, break; } + 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; -- cgit v1.2.1 From fc0afbfc7d1a49078563cc0588f342a9b7ea587c Mon Sep 17 00:00:00 2001 From: Enrico Scholz Date: Mon, 5 Sep 2022 10:56:58 +0200 Subject: tftp: make read() fail in error case when tftp transfer goes in error state e.g. due to error packets sent from the server or (unexpected) internal problems, let the read() fail instead of ignoring these errors silently and corrupting the output. Signed-off-by: Enrico Scholz Tested-by: Ahmad Fatoum Link: https://lore.barebox.org/20220905085658.3854939-4-enrico.scholz@sigma-chemnitz.de Signed-off-by: Sascha Hauer --- fs/tftp.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'fs') diff --git a/fs/tftp.c b/fs/tftp.c index ada6ad08de..e0886c49d2 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -1017,7 +1017,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize) { struct file_priv *priv = f->priv; size_t outsize = 0, now; - int ret; + int ret = 0; pr_vdebug("%s %zu\n", __func__, insize); @@ -1026,8 +1026,11 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize) outsize += now; buf += now; insize -= now; - if (priv->state == STATE_DONE) - return outsize; + + if (priv->state == STATE_DONE) { + ret = priv->err; + break; + } /* send the ACK only when fifo has been nearly depleted; else, when tftp_read() is called with small 'insize' values, it @@ -1041,9 +1044,12 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize) if (ret == TFTP_ERR_RESEND) tftp_send(priv); if (ret < 0) - return ret; + break; } + if (ret < 0) + return ret; + return outsize; } -- cgit v1.2.1