On Tue, Sep 01, 2020 at 02:30:35PM +0200, Javier Martinez Canillas wrote:
> Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regressionPlease 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 <pjo...@redhat.com> > Signed-off-by: Javier Martinez Canillas <javi...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
