Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[Ravik Hasija]

On Wed, Jun 3, 2020 at 5:55 AM Ravik Hasija 
wrote:
>
> Ramon Fried-4 wrote
> > + if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> > For servers that doesnt support windowsize option the above check could
> > result in accessing memory outside of valid range. Please check if
> (i+11)
> > < len before comparing the strings.
> This is the same handling as all other possible configurations,
> following the same code.
> I agree that this needs reworking, but I'll do it in a different patch
> all together.

Yes, the other options need to be fixed as well. However, we should fix
(i+11)




--
Sent from: http://u-boot.10912.n7.nabble.com/

Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

On Wed, Jun 3, 2020 at 5:55 AM Ravik Hasija  wrote:
>
> Ramon Fried-4 wrote
> > + if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> > For servers that doesnt support windowsize option the above check could
> > result in accessing memory outside of valid range. Please check if (i+11)
> > < len before comparing the strings.
This is the same handling as all other possible configurations,
following the same code.
I agree that this needs reworking, but I'll do it in a different patch
all together.

> >
> >
> > +
> > + if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
> > + debug("Received unexpected block: %d, expected: %d\n",
> > +   ntohs(*(__be16 *)pkt),
> > +   (ushort)(tftp_cur_block + 1));
> > + /*
> > +  * If one packet is dropped most likely
> > +  * all other buffers in the window
> > +  * that will arrive will cause a sending NACK.
> > +  * This just overwellms the server, let's just send 
> > one.
> > +  */
> > + if (tftp_last_nack != tftp_cur_block) {
> > + tftp_send();
> > + tftp_last_nack = tftp_cur_block;
> > + tftp_next_ack = (ushort)(tftp_cur_block +
> > +  tftp_windowsize);
> > + }
> > + break;
> > + }
> > +
> > + tftp_cur_block++;
> >
> > Monotonically increasing the tftp_cur_block will cause error for cases
> > where sequence number wraps around as tftp_cur_block is ulong, thus during
> > wraparound the check ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)
> > will fail and incorrectly generate ACK, and the connection will eventually
> > be terminated once the retry is exhausted. Please modulo the increment
> > with TFTP_SEQUENCE_SIZE.
True, will fix.
Thanks.
> > --
> > 2.26.2
>
> Quoted from:
> http://u-boot.10912.n7.nabble.com/PATCH-v4-net-tftp-Add-client-support-for-RFC-7440-tp412754.html
>
>
>
>
> --
> Sent from: http://u-boot.10912.n7.nabble.com/

Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[Ravik Hasija]

Ramon Fried-4 wrote
> + if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> For servers that doesnt support windowsize option the above check could
> result in accessing memory outside of valid range. Please check if (i+11)
> < len before comparing the strings.
> 
> 
> +
> + if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
> + debug("Received unexpected block: %d, expected: %d\n",
> +   ntohs(*(__be16 *)pkt),
> +   (ushort)(tftp_cur_block + 1));
> + /*
> +  * If one packet is dropped most likely
> +  * all other buffers in the window
> +  * that will arrive will cause a sending NACK.
> +  * This just overwellms the server, let's just send one.
> +  */
> + if (tftp_last_nack != tftp_cur_block) {
> + tftp_send();
> + tftp_last_nack = tftp_cur_block;
> + tftp_next_ack = (ushort)(tftp_cur_block +
> +  tftp_windowsize);
> + }
> + break;
> + }
> +
> + tftp_cur_block++;
> 
> Monotonically increasing the tftp_cur_block will cause error for cases
> where sequence number wraps around as tftp_cur_block is ulong, thus during
> wraparound the check ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)
> will fail and incorrectly generate ACK, and the connection will eventually
> be terminated once the retry is exhausted. Please modulo the increment
> with TFTP_SEQUENCE_SIZE.  
> -- 
> 2.26.2

Quoted from: 
http://u-boot.10912.n7.nabble.com/PATCH-v4-net-tftp-Add-client-support-for-RFC-7440-tp412754.html




--
Sent from: http://u-boot.10912.n7.nabble.com/

Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

On Sat, May 23, 2020 at 8:40 PM Matthias Brugger  wrote:
...
>
> I think it makes more sense to check:
> if (tftp_window_size_option > 1 && tftp_state == STATE_SEND_RRQ)
>
> Because I understand that the tftp_state will change while the
> tftp_window_size_option is set or at compile time or through the environment. 
> So
> we can save the check of the tftp_state if we have the default value.
>
> Regards,
> Matthias
>
The tftp_state can be only STATE_SEND_RRQ or STATE_SEND_WRQ and it's checked
one time per transfer, it won't change in the middle of transfer.
This code is run one time per transfer.
Thanks,
Ramon

Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[Matthias Brugger]

On 19/05/2020 21:25, Ramon Fried wrote:
> Add support for RFC 7440: "TFTP Windowsize Option".
> 
> This optional feature allows the client and server
> to negotiate a window size of consecutive blocks to send as an
> alternative for replacing the single-block lockstep schema.
> 
> windowsize can be defined statically during compilation by
> setting CONFIG_TFTP_WINDOWSIZE, or defined in runtime by
> setting an environment variable: "tftpwindowsize"
> If not defined, the windowsize is set to 1, meaning that it
> behaves as it was never defined.
> 
> Choosing the appropriate windowsize depends on the specific
> network topology, underlying NIC.
> You should test various windowsize scenarios and see which
> best work for you.
> 
> Setting a windowsize too big can actually decreases performance.
> 
> Signed-off-by: Ramon Fried 
> Reviewed-by: Marek Vasut 
> ---
> v2:
>  * Don't send windowsize option on tftpput, as it's not implemented yet.
>  * Don't send NACK for every out of order block that arrives, one nack
>is enough.
> v3:
>  * Add option CONFIG_TFTP_WINDOWSIZE to kconfig with default 1.
>  * Fixed some spelling errors.
>  * Took assignment out of a loop.
>  * simplified variable increment.
> v4:
>  * send ack for last packet, so the server can finish
>the tranfer gracefully and not in timeout.
> 
>  README  |  5 
>  net/Kconfig |  9 ++
>  net/tftp.c  | 81 +++--
>  3 files changed, 87 insertions(+), 8 deletions(-)
> 
> diff --git a/README b/README
> index be9e6391d6..686474a2f1 100644
> --- a/README
> +++ b/README
> @@ -3522,6 +3522,11 @@ List of environment variables (most likely not 
> complete):
> downloads succeed with high packet loss rates, or with
> unreliable TFTP servers or client hardware.
>  
> +  tftpwindowsize - if this is set, the value is used for TFTP's
> +   window size as described by RFC 7440.
> +   This means the count of blocks we can receive before
> +   sending ack to server.
> +
>vlan   - When set to a value < 4095 the traffic over
> Ethernet is encapsulated/received over 802.1q
> VLAN tagged frames.
> diff --git a/net/Kconfig b/net/Kconfig
> index ac6d0cf8a6..7916ae305f 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -49,4 +49,13 @@ config TFTP_BLOCKSIZE
> almost-MTU block sizes.
> You can also activate CONFIG_IP_DEFRAG to set a larger block.
>  
> +config TFTP_WINDOWSIZE
> + int "TFTP window size"
> + default 1
> + help
> +   Default TFTP window size.
> +   RFC7440 defines an optional window size of transmits,
> +   before an ack response is required.
> +   The default TFTP implementation implies a window size of 1.
> +
>  endif   # if NET
> diff --git a/net/tftp.c b/net/tftp.c
> index be24e63075..72d23e1574 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -5,7 +5,6 @@
>   * Copyright 2011 Comelit Group SpA,
>   *Luca Ceresoli 
>   */
> -
>  #include 
>  #include 
>  #include 
> @@ -95,6 +94,12 @@ static int tftp_tsize;
>  /* The number of hashes we printed */
>  static short tftp_tsize_num_hash;
>  #endif
> +/* The window size negotiated */
> +static ushorttftp_windowsize;
> +/* Next block to send ack to */
> +static ushorttftp_next_ack;
> +/* Last nack block we send */
> +static ushorttftp_last_nack;
>  #ifdef CONFIG_CMD_TFTPPUT
>  /* 1 if writing, else 0 */
>  static int   tftp_put_active;
> @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN];
>   * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg 
> file)
>   */
>  
> +/* When windowsize is defined to 1,
> + * tftp behaves the same way as it was
> + * never declared
> + */
> +#ifdef CONFIG_TFTP_WINDOWSIZE
> +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE
> +#else
> +#define TFTP_WINDOWSIZE 1
> +#endif
> +
>  static unsigned short tftp_block_size = TFTP_BLOCK_SIZE;
>  static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE;
> +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE;
>  
>  static inline int store_block(int block, uchar *src, unsigned int len)
>  {
> @@ -348,6 +364,14 @@ static void tftp_send(void)
>   /* try for more effic. blk size */
>   pkt += sprintf((char *)pkt, "blksize%c%d%c",
>   0, tftp_block_size_option, 0);
> +
> + /* try for more effic. window size.
> +  * Implemented only for tftp get.
> +  * Don't bother sending if it's 1
> +  */
> + if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1)

I think it makes more sense to check:
if (tftp_window_size_option > 1 && tftp_state == STATE_SEND_RRQ)

Because I understand that the tftp_state will change while the
tftp_window_size_option is set or at compile time or through the environment. So
we can save the check 

Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

Thanks.

On Fri, May 22, 2020 at 3:29 AM rahasij  wrote:
>
> Ramon Fried-4 wrote
> > + if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> > + tftp_windowsize =
> > + simple_strtoul((char *)pkt + i + 11,
> > +NULL, 10);
> > + debug("windowsize = %s, %d\n",
> > +   (char *)pkt + i + 11, tftp_windowsize);
> > + }
> > +
> >   }
> > --
> > 2.26.2
>
> As per RFC2347, the option string is case insensitive. I fixed this for
> other options in following patch
>
> https://lists.denx.de/pipermail/u-boot/2020-May/412472.html
>
> Please use strcasecmp() instead of strcmp().
>
> As per RFC7440, the value received from server should be less than or equal
> to the value proposed by client . This check should be added here, and error
> packet must be generated in case of failure.
>
> Above patch implements ERR pkt generation and should be applied first.
>
>
>
>
> --
> Sent from: http://u-boot.10912.n7.nabble.com/

Re: [PATCH v4] net: tftp: Add client support for RFC 7440

[rahasij]

Ramon Fried-4 wrote
> + if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> + tftp_windowsize =
> + simple_strtoul((char *)pkt + i + 11,
> +NULL, 10);
> + debug("windowsize = %s, %d\n",
> +   (char *)pkt + i + 11, tftp_windowsize);
> + }
> +
>   }
> -- 
> 2.26.2

As per RFC2347, the option string is case insensitive. I fixed this for
other options in following patch

https://lists.denx.de/pipermail/u-boot/2020-May/412472.html

Please use strcasecmp() instead of strcmp().

As per RFC7440, the value received from server should be less than or equal
to the value proposed by client . This check should be added here, and error
packet must be generated in case of failure. 

Above patch implements ERR pkt generation and should be applied first.




--
Sent from: http://u-boot.10912.n7.nabble.com/