* [PATCH 0/2] efinet: correct closing of SNP protocol
@ 2021-10-06 8:30 Heinrich Schuchardt
2021-10-06 8:30 ` [PATCH 1/2] " Heinrich Schuchardt
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-10-06 8:30 UTC (permalink / raw)
To: Daniel Kiper
Cc: Andrei Borzenkov, Vladimir 'phcoder' Serbinenko,
Nikita Ermakov, Ard Biesheuvel, Atish Patra, Leif Lindholm,
Andreas Schwab, GRUB development mailing list,
Heinrich Schuchardt
In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for
the initial ramdisk it was observed that opening the SNP protocol failed
(https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html).
This is due to an incorrect call to CloseProtocol().
This is corrected in the first patch.
The second patch provides a new function grub_efi_close_protocol() to
simplify the coding.
Heinrich Schuchardt (2):
efinet: correct closing of SNP protocol
efi: library function grub_efi_close_protocol()
grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
grub-core/net/drivers/efi/efinet.c | 30 +++++++++++++++++++++---------
include/grub/efi/efi.h | 3 +++
3 files changed, 42 insertions(+), 9 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] efinet: correct closing of SNP protocol 2021-10-06 8:30 [PATCH 0/2] efinet: correct closing of SNP protocol Heinrich Schuchardt @ 2021-10-06 8:30 ` Heinrich Schuchardt 2021-11-23 17:52 ` Daniel Kiper 2021-10-06 8:30 ` [PATCH 2/2] efi: library function grub_efi_close_protocol() Heinrich Schuchardt 2021-10-06 14:28 ` [PATCH 0/2] efinet: correct closing of SNP protocol Andreas Schwab 2 siblings, 1 reply; 6+ messages in thread From: Heinrich Schuchardt @ 2021-10-06 8:30 UTC (permalink / raw) To: Daniel Kiper Cc: Andrei Borzenkov, Vladimir 'phcoder' Serbinenko, Nikita Ermakov, Ard Biesheuvel, Atish Patra, Leif Lindholm, Andreas Schwab, GRUB development mailing list, Heinrich Schuchardt In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for the initial ramdisk it was observed that opening the SNP protocol failed. https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html This is due to an incorrect call to CloseProtocol(). The first parameter of CloseProtocol() is the handle, not the interface. We call OpenProtocol() with ControllerHandle = NULL. Hence we must also call CloseProtcol with ControllerHandel = NULL. Each call of OpenProtocol() for the same network card handle is expected to return the same interface pointer. If we want to close the protocol which we opened non-exclusively when searching for a card, we have to do this before opening the protocol exclusively. As there is no guarantee that we successfully open the protocol add checks in the transmit and receive functions. Reported-by: Andreas Schwab <schwab@linux-m68k.org> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- grub-core/net/drivers/efi/efinet.c | 32 ++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c index 5388f952b..3f2ff03f5 100644 --- a/grub-core/net/drivers/efi/efinet.c +++ b/grub-core/net/drivers/efi/efinet.c @@ -39,6 +39,8 @@ send_card_buffer (struct grub_net_card *dev, grub_uint64_t limit_time = grub_get_time_ms () + 4000; void *txbuf; + if (!net) + return grub_error (GRUB_ERR_IO, N_("couldn't send network packet")); if (dev->txbusy) while (1) { @@ -101,6 +103,9 @@ get_card_packet (struct grub_net_card *dev) struct grub_net_buff *nb; int i; + if (!net) + return NULL; + for (i = 0; i < 2; i++) { if (!dev->rcvbuf) @@ -148,11 +153,23 @@ open_card (struct grub_net_card *dev) { grub_efi_simple_network_t *net; - /* Try to reopen SNP exlusively to close any active MNP protocol instance - that may compete for packet polling + if (dev->efi_net) + { + efi_call_4 (grub_efi_system_table->boot_services->close_protocol, + dev->efi_handle, &net_io_guid, + grub_efi_image_handle, 0); + dev->efi_net = NULL; + } + /* + * Try to reopen SNP exlusively to close any active MNP protocol instance + * that may compete for packet polling */ net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid, GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE); + /* If exclusively fails, try non-exclusively. */ + if (!net) + net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid, + GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL); if (net) { if (net->mode->state == GRUB_EFI_NETWORK_STOPPED @@ -192,13 +209,12 @@ open_card (struct grub_net_card *dev) efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL); } - efi_call_4 (grub_efi_system_table->boot_services->close_protocol, - dev->efi_net, &net_io_guid, - grub_efi_image_handle, dev->efi_handle); dev->efi_net = net; + } else { + return grub_error (GRUB_ERR_NET_NO_CARD, "%s: can't open protocol", + dev->name); } - /* If it failed we just try to run as best as we can */ return GRUB_ERR_NONE; } @@ -208,8 +224,8 @@ close_card (struct grub_net_card *dev) efi_call_1 (dev->efi_net->shutdown, dev->efi_net); efi_call_1 (dev->efi_net->stop, dev->efi_net); efi_call_4 (grub_efi_system_table->boot_services->close_protocol, - dev->efi_net, &net_io_guid, - grub_efi_image_handle, dev->efi_handle); + dev->efi_handle, &net_io_guid, + grub_efi_image_handle, 0); } static struct grub_net_card_driver efidriver = -- 2.32.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] efinet: correct closing of SNP protocol 2021-10-06 8:30 ` [PATCH 1/2] " Heinrich Schuchardt @ 2021-11-23 17:52 ` Daniel Kiper 0 siblings, 0 replies; 6+ messages in thread From: Daniel Kiper @ 2021-11-23 17:52 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Andrei Borzenkov, Vladimir 'phcoder' Serbinenko, Nikita Ermakov, Ard Biesheuvel, Atish Patra, Leif Lindholm, Andreas Schwab, GRUB development mailing list First of all, sorry for late reply but I am busy... On Wed, Oct 06, 2021 at 10:30:42AM +0200, Heinrich Schuchardt wrote: > In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for > the initial ramdisk it was observed that opening the SNP protocol failed. > https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html > This is due to an incorrect call to CloseProtocol(). > > The first parameter of CloseProtocol() is the handle, not the interface. > > We call OpenProtocol() with ControllerHandle = NULL. Hence we must also > call CloseProtcol with ControllerHandel = NULL. > > Each call of OpenProtocol() for the same network card handle is expected to > return the same interface pointer. If we want to close the protocol which > we opened non-exclusively when searching for a card, we have to do this > before opening the protocol exclusively. > > As there is no guarantee that we successfully open the protocol add checks > in the transmit and receive functions. > > Reported-by: Andreas Schwab <schwab@linux-m68k.org> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > grub-core/net/drivers/efi/efinet.c | 32 ++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c > index 5388f952b..3f2ff03f5 100644 > --- a/grub-core/net/drivers/efi/efinet.c > +++ b/grub-core/net/drivers/efi/efinet.c > @@ -39,6 +39,8 @@ send_card_buffer (struct grub_net_card *dev, > grub_uint64_t limit_time = grub_get_time_ms () + 4000; > void *txbuf; > > + if (!net) if (net == NULL) > + return grub_error (GRUB_ERR_IO, N_("couldn't send network packet")); Could you display more specific error here? > if (dev->txbusy) > while (1) > { > @@ -101,6 +103,9 @@ get_card_packet (struct grub_net_card *dev) > struct grub_net_buff *nb; > int i; > > + if (!net) if (net == NULL) > + return NULL; > + > for (i = 0; i < 2; i++) > { > if (!dev->rcvbuf) > @@ -148,11 +153,23 @@ open_card (struct grub_net_card *dev) > { > grub_efi_simple_network_t *net; > > - /* Try to reopen SNP exlusively to close any active MNP protocol instance > - that may compete for packet polling > + if (dev->efi_net) if (dev->efi_net != NULL) ... and please fix this in the other places in the patch(es) too... > + { > + efi_call_4 (grub_efi_system_table->boot_services->close_protocol, > + dev->efi_handle, &net_io_guid, > + grub_efi_image_handle, 0); s/0/NULL/ > + dev->efi_net = NULL; > + } > + /* > + * Try to reopen SNP exlusively to close any active MNP protocol instance > + * that may compete for packet polling > */ > net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid, > GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE); > + /* If exclusively fails, try non-exclusively. */ > + if (!net) > + net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid, > + GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL); I am not convinced it is correct. Please take a look at send_card_buffer()... 46 st = efi_call_3 (net->get_status, net, 0, &txbuf); 47 if (st != GRUB_EFI_SUCCESS) 48 return grub_error (GRUB_ERR_IO, 49 N_("couldn't send network packet")); 50 /* 51 Some buggy firmware could return an arbitrary address instead of the 52 txbuf address we trasmitted, so just check that txbuf is non NULL 53 for success. This is ok because we open the SNP protocol in 54 exclusive mode so we know we're the only ones transmitting on this Here ---> ^^^^^^^^^ 55 box and since we only transmit one packet at a time we know our 56 transmit was successfull. 57 */ 58 if (txbuf) 59 { 60 dev->txbusy = 0; 61 break; 62 } > if (net) > { > if (net->mode->state == GRUB_EFI_NETWORK_STOPPED > @@ -192,13 +209,12 @@ open_card (struct grub_net_card *dev) > efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL); > } > > - efi_call_4 (grub_efi_system_table->boot_services->close_protocol, > - dev->efi_net, &net_io_guid, > - grub_efi_image_handle, dev->efi_handle); > dev->efi_net = net; > + } else { > + return grub_error (GRUB_ERR_NET_NO_CARD, "%s: can't open protocol", > + dev->name); > } > > - /* If it failed we just try to run as best as we can */ > return GRUB_ERR_NONE; > } > > @@ -208,8 +224,8 @@ close_card (struct grub_net_card *dev) > efi_call_1 (dev->efi_net->shutdown, dev->efi_net); > efi_call_1 (dev->efi_net->stop, dev->efi_net); > efi_call_4 (grub_efi_system_table->boot_services->close_protocol, > - dev->efi_net, &net_io_guid, > - grub_efi_image_handle, dev->efi_handle); > + dev->efi_handle, &net_io_guid, > + grub_efi_image_handle, 0); s/0/NULL/ Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] efi: library function grub_efi_close_protocol() 2021-10-06 8:30 [PATCH 0/2] efinet: correct closing of SNP protocol Heinrich Schuchardt 2021-10-06 8:30 ` [PATCH 1/2] " Heinrich Schuchardt @ 2021-10-06 8:30 ` Heinrich Schuchardt 2021-11-23 17:59 ` Daniel Kiper 2021-10-06 14:28 ` [PATCH 0/2] efinet: correct closing of SNP protocol Andreas Schwab 2 siblings, 1 reply; 6+ messages in thread From: Heinrich Schuchardt @ 2021-10-06 8:30 UTC (permalink / raw) To: Daniel Kiper Cc: Andrei Borzenkov, Vladimir 'phcoder' Serbinenko, Nikita Ermakov, Ard Biesheuvel, Atish Patra, Leif Lindholm, Andreas Schwab, GRUB development mailing list, Heinrich Schuchardt Create a library function for CloseProtocol() and use it for the SNP driver. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- grub-core/kern/efi/efi.c | 18 ++++++++++++++++++ grub-core/net/drivers/efi/efinet.c | 8 ++------ include/grub/efi/efi.h | 3 +++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c index 8cff7be02..1d331d858 100644 --- a/grub-core/kern/efi/efi.c +++ b/grub-core/kern/efi/efi.c @@ -117,6 +117,24 @@ grub_efi_open_protocol (grub_efi_handle_t handle, return interface; } +grub_efi_status_t +grub_efi_close_protocol (grub_efi_handle_t handle, + grub_efi_guid_t *protocol) +{ + grub_efi_boot_services_t *b; + grub_efi_status_t status; + + + b = grub_efi_system_table->boot_services; + status = efi_call_4 (b->close_protocol, + handle, + protocol, + grub_efi_image_handle, + 0); + + return status; +} + int grub_efi_set_text_mode (int on) { diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c index 3f2ff03f5..2febbf84a 100644 --- a/grub-core/net/drivers/efi/efinet.c +++ b/grub-core/net/drivers/efi/efinet.c @@ -155,9 +155,7 @@ open_card (struct grub_net_card *dev) if (dev->efi_net) { - efi_call_4 (grub_efi_system_table->boot_services->close_protocol, - dev->efi_handle, &net_io_guid, - grub_efi_image_handle, 0); + grub_efi_close_protocol (dev->efi_handle, &net_io_guid); dev->efi_net = NULL; } /* @@ -223,9 +221,7 @@ close_card (struct grub_net_card *dev) { efi_call_1 (dev->efi_net->shutdown, dev->efi_net); efi_call_1 (dev->efi_net->stop, dev->efi_net); - efi_call_4 (grub_efi_system_table->boot_services->close_protocol, - dev->efi_handle, &net_io_guid, - grub_efi_image_handle, 0); + grub_efi_close_protocol (dev->efi_handle, &net_io_guid); } static struct grub_net_card_driver efidriver = diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h index 83d958f99..12d041f2e 100644 --- a/include/grub/efi/efi.h +++ b/include/grub/efi/efi.h @@ -35,6 +35,9 @@ EXPORT_FUNC(grub_efi_locate_handle) (grub_efi_locate_search_type_t search_type, void *EXPORT_FUNC(grub_efi_open_protocol) (grub_efi_handle_t handle, grub_efi_guid_t *protocol, grub_efi_uint32_t attributes); +grub_efi_status_t +EXPORT_FUNC(grub_efi_close_protocol) (grub_efi_handle_t handle, + grub_efi_guid_t *protocol); int EXPORT_FUNC(grub_efi_set_text_mode) (int on); void EXPORT_FUNC(grub_efi_stall) (grub_efi_uintn_t microseconds); void * -- 2.32.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] efi: library function grub_efi_close_protocol() 2021-10-06 8:30 ` [PATCH 2/2] efi: library function grub_efi_close_protocol() Heinrich Schuchardt @ 2021-11-23 17:59 ` Daniel Kiper 0 siblings, 0 replies; 6+ messages in thread From: Daniel Kiper @ 2021-11-23 17:59 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Andrei Borzenkov, Vladimir 'phcoder' Serbinenko, Nikita Ermakov, Ard Biesheuvel, Atish Patra, Leif Lindholm, Andreas Schwab, GRUB development mailing list On Wed, Oct 06, 2021 at 10:30:43AM +0200, Heinrich Schuchardt wrote: > Create a library function for CloseProtocol() and use it for the SNP > driver. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > grub-core/kern/efi/efi.c | 18 ++++++++++++++++++ > grub-core/net/drivers/efi/efinet.c | 8 ++------ > include/grub/efi/efi.h | 3 +++ > 3 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c > index 8cff7be02..1d331d858 100644 > --- a/grub-core/kern/efi/efi.c > +++ b/grub-core/kern/efi/efi.c > @@ -117,6 +117,24 @@ grub_efi_open_protocol (grub_efi_handle_t handle, > return interface; > } > > +grub_efi_status_t > +grub_efi_close_protocol (grub_efi_handle_t handle, > + grub_efi_guid_t *protocol) grub_efi_close_protocol (grub_efi_handle_t handle, grub_efi_guid_t *protocol) > +{ > + grub_efi_boot_services_t *b; > + grub_efi_status_t status; > + > + Please drop this redundant empty line... > + b = grub_efi_system_table->boot_services; I think you could do this above: grub_efi_boot_services_t *b = grub_efi_system_table->boot_services; > + status = efi_call_4 (b->close_protocol, > + handle, > + protocol, > + grub_efi_image_handle, > + 0); status = efi_call_4 (b->close_protocol, handle, protocol, grub_efi_image_handle, NULL); I am OK with lines a bit longer than 80 chars. Otherwise LGTM. Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] efinet: correct closing of SNP protocol 2021-10-06 8:30 [PATCH 0/2] efinet: correct closing of SNP protocol Heinrich Schuchardt 2021-10-06 8:30 ` [PATCH 1/2] " Heinrich Schuchardt 2021-10-06 8:30 ` [PATCH 2/2] efi: library function grub_efi_close_protocol() Heinrich Schuchardt @ 2021-10-06 14:28 ` Andreas Schwab 2 siblings, 0 replies; 6+ messages in thread From: Andreas Schwab @ 2021-10-06 14:28 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Daniel Kiper, Andrei Borzenkov, Vladimir 'phcoder' Serbinenko, Nikita Ermakov, Ard Biesheuvel, Atish Patra, Leif Lindholm, GRUB development mailing list On Okt 06 2021, Heinrich Schuchardt wrote: > In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for > the initial ramdisk it was observed that opening the SNP protocol failed > (https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html). > This is due to an incorrect call to CloseProtocol(). > > This is corrected in the first patch. > > The second patch provides a new function grub_efi_close_protocol() to > simplify the coding. Sucessfully tested with https://download.opensuse.org/ports/riscv/tumbleweed/iso/openSUSE-Tumbleweed-NET-riscv64-Media.iso on the Hifive Unmatched. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-23 17:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-06 8:30 [PATCH 0/2] efinet: correct closing of SNP protocol Heinrich Schuchardt 2021-10-06 8:30 ` [PATCH 1/2] " Heinrich Schuchardt 2021-11-23 17:52 ` Daniel Kiper 2021-10-06 8:30 ` [PATCH 2/2] efi: library function grub_efi_close_protocol() Heinrich Schuchardt 2021-11-23 17:59 ` Daniel Kiper 2021-10-06 14:28 ` [PATCH 0/2] efinet: correct closing of SNP protocol Andreas Schwab
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.