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

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

[Javier Martinez Canillas]

Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression
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")
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)
-- 
2.28.0

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

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

[Javier Martinez Canillas]

Hello Daniel,

Thanks for the review.

On 9/7/20 9:36 PM, Daniel Kiper wrote:
> 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.
>

Sure, I can do that but I wonder why you don't want the quotes.
That's the convention used in many other projects.

[snip] 

>>
>> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
> 
> Please drop this line.
>

Same question here. I think is important information, specially for
downstream since they could allow people to decide whether they need
to backport this patch or not.

[snip] 

>> +       */
>> +      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.
>

True, thanks for pointing it out.
 
> However, why do not do "((grub_uint16_t) (data->block + 1))" instead of
> "& BLK_MASK" in general?
> 

Indeed. I'll change that and post a v2.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

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

[Daniel Kiper]

Hey Javier,

On Wed, Sep 09, 2020 at 12:47:20PM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> Thanks for the review.
>
> On 9/7/20 9:36 PM, Daniel Kiper wrote:
> > 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.
> >
>
> Sure, I can do that but I wonder why you don't want the quotes.
> That's the convention used in many other projects.

I think quotes are superfluous if you have parentheses.

> [snip]
>
> >>
> >> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
> >
> > Please drop this line.
> >
>
> Same question here. I think is important information, specially for
> downstream since they could allow people to decide whether they need
> to backport this patch or not.

You duplicate the information which is above. Additionally, IMO "Fixes:"
should contain bug number, CVE number, link to the bug, etc. not the
commit id.

Daniel

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

[Alexey Makhalov]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]


> On Sep 9, 2020, at 4:12 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> Hey Javier,
> 
> On Wed, Sep 09, 2020 at 12:47:20PM +0200, Javier Martinez Canillas wrote:
>> Hello Daniel,
>> 
>> Thanks for the review.
>> 
>> On 9/7/20 9:36 PM, Daniel Kiper wrote:
>>> 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.
>>> 
>> 
>> Sure, I can do that but I wonder why you don't want the quotes.
>> That's the convention used in many other projects.
> 
> I think quotes are superfluous if you have parentheses.
> 
>> [snip]
>> 
>>>> 
>>>> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
>>> 
>>> Please drop this line.
>>> 
>> 
>> Same question here. I think is important information, specially for
>> downstream since they could allow people to decide whether they need
>> to backport this patch or not.
> 
> You duplicate the information which is above. Additionally, IMO "Fixes:"
> should contain bug number, CVE number, link to the bug, etc. not the
> commit id.

I think “Fixes: commit id” should remain in place. It provides direct information
from what commit the bug existed in case of regression.

—Alexey


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Javier Martinez Canillas]

On 9/10/20 12:00 AM, Alexey Makhalov wrote:

[snip]

>>>>> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
>>>>
>>>> Please drop this line.
>>>>
>>>
>>> Same question here. I think is important information, specially for
>>> downstream since they could allow people to decide whether they need
>>> to backport this patch or not.
>>
>> You duplicate the information which is above. Additionally, IMO "Fixes:"
>> should contain bug number, CVE number, link to the bug, etc. not the
>> commit id.
> 
> I think “Fixes: commit id” should remain in place. It provides direct information
> from what commit the bug existed in case of regression.
>

Yes, I think the same. For example I usually do git log --grep="Fixes:", but
don't have a strong opinion and I'm OK with dropping it if Daniel prefer that.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

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

[Daniel Kiper]

On Thu, Sep 10, 2020 at 12:07:42PM +0200, Javier Martinez Canillas wrote:
> On 9/10/20 12:00 AM, Alexey Makhalov wrote:
>
> [snip]
>
> >>>>> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
> >>>>
> >>>> Please drop this line.
> >>>>
> >>>
> >>> Same question here. I think is important information, specially for
> >>> downstream since they could allow people to decide whether they need
> >>> to backport this patch or not.
> >>
> >> You duplicate the information which is above. Additionally, IMO "Fixes:"
> >> should contain bug number, CVE number, link to the bug, etc. not the
> >> commit id.
> >
> > I think “Fixes: commit id” should remain in place. It provides direct information
> > from what commit the bug existed in case of regression.
> >
>
> Yes, I think the same. For example I usually do git log --grep="Fixes:", but
> don't have a strong opinion and I'm OK with dropping it if Daniel prefer that.

If more people like it I am not going to object so strongly...

Daniel