Re: [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets

[Andre Przywara]

On Fri, 8 Mar 2019 13:01:33 +0100
Daniel Kiper <address@hidden> wrote:

> On Thu, Mar 07, 2019 at 03:14:14PM +0000, Andre Przywara wrote:
> > From: Andrei Borzenkov <address@hidden>
> >
> > In respone to a BOOTREQUEST packet a BOOTP server would answer with a
> > BOOTREPLY packet, which ends the conversation for good.
> > DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
> > which has to be answered with REQUEST by the client again, only to be
> > completed by an ACKNOWLEDGE packet from the server.
> >
> > Teach the grub_net_process_dhcp() function to deal with OFFER packets,
> > and treat ACK packets the same es BOOTREPLY packets.
> >
> > Signed-off-by: Andre Przywara <address@hidden>
> > Reviewed-by: Daniel Kiper <address@hidden>  
> 
> This patch has changed and you retained my RB and not explained why it
> has changed. Could you tell us what has happened? Next time please drop
> RB if you change the code significantly.

Mmh, weird. I didn't touch this patch at all, that's why just added your
R-B to the commit message. I think what happened is that the subtle change
in patch 05/10 (just removing the not needed brackets, as per your comment
on v2 4/9), caused the *diff* to be vastly different. I remember fixing it
up in the "git rebase -i" process. I just did a:
$ diff -u <(git show dhcp-v2~2:grub-core/net/bootp.c) <(git show 
dhcp-v3~2:grub-core/net/bootp.c)
and that showed only unrelated changes.
So the resulting code change is actually identical, it's just that diff
took a different approach at expressing that.

Sorry for the confusion, hope that your R-b: still stands.

Cheers,
Andre.

> Hmmm... It seems to me that at least partially this is due to change in
> patch #5.
> 
> Daniel
> 
> > ---
> >  grub-core/net/bootp.c | 78 +++++++++++++++++++++++++++++++++++--------
> >  include/grub/net.h    |  5 +++
> >  2 files changed, 70 insertions(+), 13 deletions(-)
> >
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index bfde6ef10..d0482a7d5 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -30,6 +30,21 @@ enum
> >    GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> >    GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> >  };
> > +enum
> > +{
> > +  GRUB_DHCP_MESSAGE_UNKNOWN,
> > +  GRUB_DHCP_MESSAGE_DISCOVER,
> > +  GRUB_DHCP_MESSAGE_OFFER,
> > +  GRUB_DHCP_MESSAGE_REQUEST,
> > +  GRUB_DHCP_MESSAGE_DECLINE,
> > +  GRUB_DHCP_MESSAGE_ACK,
> > +  GRUB_DHCP_MESSAGE_NAK,
> > +  GRUB_DHCP_MESSAGE_RELEASE,
> > +  GRUB_DHCP_MESSAGE_INFORM,
> > +};
> > +
> > +/* Max timeout when waiting for BOOTP/DHCP reply */
> > +#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
> >
> >  #define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
> >
> > @@ -443,22 +458,59 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
> >    struct grub_net_card *card = iface->card;
> >    const struct grub_net_bootp_packet *bp = (const struct 
> > grub_net_bootp_packet *) nb->data;
> >    grub_size_t size = nb->tail - nb->data;
> > +  const grub_uint8_t *opt;
> > +  grub_uint8_t opt_len, type;
> > +  grub_uint32_t srv_id = 0;
> > +
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_MESSAGE_TYPE, &opt_len);
> > +  if (opt && opt_len == 1)
> > +    type = *opt;
> > +  else
> > +    type = GRUB_DHCP_MESSAGE_UNKNOWN;
> > +
> > +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_SERVER_IDENTIFIER, 
> > &opt_len);
> > +  if (opt && opt_len == sizeof (srv_id))
> > +    srv_id = grub_get_unaligned32 (opt);
> >
> > -  name = grub_xasprintf ("%s:dhcp", card->name);
> > -  if (!name)
> > +  /*
> > +   * If we received BOOTP reply or DHCPACK, proceed with configuration.
> > +   * Otherwise store offered address and server id for later processing
> > +   * of DHCPACK.
> > +   * xid and srv_id are stored in network order so do not need conversion.
> > +   */
> > +  if ((!iface->srv_id && type == GRUB_DHCP_MESSAGE_UNKNOWN)
> > +      || (iface->srv_id && type == GRUB_DHCP_MESSAGE_ACK
> > +     && bp->ident == iface->xid
> > +     && srv_id == iface->srv_id))
> >      {
> > -      grub_print_error ();
> > -      return;
> > +      name = grub_xasprintf ("%s:dhcp", card->name);
> > +      if (!name)
> > +   {
> > +     grub_print_error ();
> > +     return;
> > +   }
> > +      grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> > +      grub_free (name);
> > +      if (grub_errno)
> > +   grub_print_error ();
> > +      else
> > +   grub_net_network_level_interface_unregister (iface);
> > +    }
> > +  else if (!iface->srv_id && type == GRUB_DHCP_MESSAGE_OFFER && srv_id)
> > +    {
> > +      iface->srv_id = srv_id;
> > +      iface->my_ip = bp->your_ip;
> > +      /* Reset retransmission timer */
> > +      iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> > +    }
> > +  else if (iface->srv_id && type == GRUB_DHCP_MESSAGE_NAK
> > +      && bp->ident == iface->xid
> > +      && srv_id == iface->srv_id)
> > +    {
> > +      iface->xid = iface->srv_id = iface->my_ip = 0;
> > +      /* Reset retransmission timer */
> > +      iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> >      }
> > -  grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> > -  grub_free (name);
> > -  if (grub_errno)
> > -    grub_print_error ();
> > -  else
> > -    if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 
> > 0 &&
> > -   grub_memcmp (iface->name + grub_strlen (card->name),
> > -                     ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
> > -      grub_net_network_level_interface_unregister (iface);
> >  }
> >
> >  static char
> > diff --git a/include/grub/net.h b/include/grub/net.h
> > index f7b546446..68a9f1311 100644
> > --- a/include/grub/net.h
> > +++ b/include/grub/net.h
> > @@ -292,6 +292,9 @@ struct grub_net_network_level_interface
> >    struct grub_net_bootp_packet *dhcp_ack;
> >    grub_size_t dhcp_acklen;
> >    grub_uint16_t vlantag;
> > +  grub_uint32_t xid;      /* DHCPv4 transaction id */
> > +  grub_uint32_t srv_id;   /* DHCPv4 server_identifier */
> > +  grub_uint32_t my_ip;    /* DHCPv4 offered IP address */
> >    unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
> >    unsigned dhcp_tmo;      /* DHCPv4 current retransmission timeout */
> >    void *data;
> > @@ -460,6 +463,8 @@ enum
> >      GRUB_NET_BOOTP_ROOT_PATH = 0x11,
> >      GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
> >      GRUB_NET_DHCP_OVERLOAD = 52,
> > +    GRUB_NET_DHCP_MESSAGE_TYPE = 53,
> > +    GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
> >      GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
> >      GRUB_NET_DHCP_BOOTFILE_NAME = 67,
> >      GRUB_NET_BOOTP_END = 0xff
> > --
> > 2.17.1