Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

[Daniel Kiper]

On Tue, Sep 01, 2020 at 02:30:35PM +0200, Javier Martinez Canillas wrote:
> Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression

Please drop the quotes.

> when fetching files over TFTP whose size is bigger than 65535 * block size.
>
>   grub> linux /images/pxeboot/vmlinuz
>   grub> echo $?
>   0
>   grub> initrd /images/pxeboot/initrd.img
>   error: timeout reading '/images/pxeboot/initrd.img'.
>   grub> echo $?
>   28
>
> It is caused by the block number counter being a 16-bit field, which leads
> to a maximum file size of ((1 << 16) - 1) * block size. Because GRUB sets
> the block size to 1024 octets (by using the TFTP Blocksize Option from RFC
> 2348 [0]), the maximum file size that can be transferred is 67107840 bytes.
>
> The TFTP PROTOCOL (REVISION 2) RFC 1350 [1] does not mention what a client
> should do when a file size is bigger than the maximum, but most TFTP hosts
> support the block number counter to be rolled over. That is, acking a data
> packet with a block number of 0 is taken as if the 65356th block was acked.
>
> It was working before because the block counter roll-over was happening due
> an overflow. But that got fixed by the mentioned commit, which led to the
> regression when attempting to fetch files larger than the maximum size.
>
> To allow TFTP file transfers of unlimited size again, re-introduce a block
> counter roll-over so the data packets are acked preventing the timeouts.
>
> [0]: https://tools.ietf.org/html/rfc2348
> [1]: https://tools.ietf.org/html/rfc1350
>
> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")

Please drop this line.

> Suggested-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>
> ---
>
>  grub-core/net/tftp.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
> index 644135caf46..ba90c2bc45a 100644
> --- a/grub-core/net/tftp.c
> +++ b/grub-core/net/tftp.c
> @@ -29,6 +29,8 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> +#define BLK_MASK ((1 << 16) - 1)
> +
>  /* IP port for the MTFTP server used for Intel's PXE */
>  enum
>    {
> @@ -183,8 +185,19 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__ 
> ((unused)),
>         return GRUB_ERR_NONE;
>       }
>
> -      /* Ack old/retransmitted block. */
> -      if (grub_be_to_cpu16 (tftph->u.data.block) < data->block + 1)
> +      /*
> +       * Ack old/retransmitted block.
> +       *
> +       * The block number is a 16-bit counter, thus the maximum file size 
> that
> +       * could be transfered is 65535 * block size. Most TFTP hosts support 
> to
> +       * roll-over the block counter to allow unlimited transfer file size.
> +       *
> +       * This behavior is not defined in the RFC 1350 [0] but is implemented 
> by
> +       * most TFTP clients and hosts.
> +       *
> +       * [0]: https://tools.ietf.org/html/rfc1350
> +       */
> +      if (grub_be_to_cpu16 (tftph->u.data.block) < ((data->block + 1) & 
> BLK_MASK))
>       ack (data, grub_be_to_cpu16 (tftph->u.data.block));
>        /* Ignore unexpected block. */
>        else if (grub_be_to_cpu16 (tftph->u.data.block) > data->block + 1)

I think the fix is incomplete. You should change the line above too.

However, why do not do "((grub_uint16_t) (data->block + 1))" instead of
"& BLK_MASK" in general?

Daniel