[PATCH 0/3] log: Fix segfault in sandbox when LOG_LEVEL_DEFAULT >= LOGL_DEBUG

[PATCH 0/3] log: Fix segfault in sandbox when LOG_LEVEL_DEFAULT >= LOGL_DEBUG

[Sean Anderson]

Since the syslog feature has been introduced, sandbox no longer boots when
LOG_LEVEL_DEFAULT prints cdebug messages. This is because it tries to call
net_init before initf_dm.


Sean Anderson (3):
  dev: Disambiguate errors in uclass_find
  net: Expose some errors generated in net_init
  log: syslog: Handle errors in net_init

 common/log_syslog.c   |  4 +++-
 drivers/core/uclass.c | 16 +++++++++++++++-
 include/net.h         |  2 +-
 net/eth-uclass.c      |  3 +++
 net/net.c             | 15 +++++++++++----
 test/dm/core.c        |  2 +-
 test/dm/test-main.c   |  2 +-
 7 files changed, 35 insertions(+), 9 deletions(-)

-- 
2.28.0

[PATCH 1/3] dev: Disambiguate errors in uclass_find

[Sean Anderson]

There are two cases where uclass_find can return an error. The second is
when the uclass has not yet been init'd. The first is when the driver model
has not been init'd (or has been only partially init'd) and there is no
root uclass driver.

If LOG_SYSLOG is enabled and LOG_DEFAULT_LEVEL is set to LOGL_DEBUG or
higher, log_syslog_emit will try to call net_init before initf_dm has been
called. This in turn (eventually) calls uclass_get for UCLASS_ETH.

If the second error occurs, uclass_get should call uclass_add to create the
uclass. If the first error occurs, then uclass_get should not call
uclass_add (because the uclass list has not been initialized). However, the
first error is expected when calling uclass_get for UCLASS_ROOT, so we add
an exception.

There are several alternative approaches to this patch. One option would be
to duplicate the check against gd->dm_root in uclass_get and not change the
behavior of uclass_find. I did not choose this approach because I think it
it is less clear than the patch as-is. However, that is certainly
subjective, and there is no other technical reason to do it this way.

Another approach would have been to change log_syslog_emit to abort if it
is called too early. I did not choose this approach because the check in
uclass_find to see if gd->dm_root exists implies that functions in its
file are allowed to be called at any time. There is an argument to be made
that callers should ensure that they don't call certain functions too
early. I think it is better to code defensively in these circumstances to
make it easier to discover such errors.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/core/uclass.c | 16 +++++++++++++++-
 test/dm/core.c        |  2 +-
 test/dm/test-main.c   |  2 +-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index c3f1b73cd6..2e098034c9 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -19,6 +19,7 @@
 #include <dm/uclass.h>
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
+#include <linux/err.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -27,7 +28,7 @@ struct uclass *uclass_find(enum uclass_id key)
 	struct uclass *uc;
 
 	if (!gd->dm_root)
-		return NULL;
+		return ERR_PTR(-EAGAIN);
 	/*
 	 * TODO(sjg at chromium.org): Optimise this, perhaps moving the found
 	 * node to the start of the list, or creating a linear array mapping
@@ -144,6 +145,19 @@ int uclass_get(enum uclass_id id, struct uclass **ucp)
 
 	*ucp = NULL;
 	uc = uclass_find(id);
+	if (IS_ERR(uc)) {
+		/*
+		 * If we're getting the root uclass, then uclass_find will fail
+		 * with -EAGAIN (since it thinks there's no uclass list to
+		 * search), but we need to add it anyway (since otherwise it
+		 * would never be created).
+		 */
+		if (PTR_ERR(uc) == -EAGAIN && id == UCLASS_ROOT)
+			uc = NULL;
+		else
+			return PTR_ERR(uc);
+	}
+
 	if (!uc)
 		return uclass_add(id, ucp);
 	*ucp = uc;
diff --git a/test/dm/core.c b/test/dm/core.c
index 8ed5bf7370..a4e0ac06f9 100644
--- a/test/dm/core.c
+++ b/test/dm/core.c
@@ -733,7 +733,7 @@ static int dm_test_uclass_before_ready(struct unit_test_state *uts)
 	gd->dm_root_f = NULL;
 	memset(&gd->uclass_root, '\0', sizeof(gd->uclass_root));
 
-	ut_asserteq_ptr(NULL, uclass_find(UCLASS_TEST));
+	ut_asserteq_ptr(ERR_PTR(-EAGAIN), uclass_find(UCLASS_TEST));
 
 	return 0;
 }
diff --git a/test/dm/test-main.c b/test/dm/test-main.c
index 38b7b1481a..6ff07a5d32 100644
--- a/test/dm/test-main.c
+++ b/test/dm/test-main.c
@@ -70,7 +70,7 @@ static int dm_test_destroy(struct unit_test_state *uts)
 		 * check that here before we call uclass_find_device().
 		 */
 		uc = uclass_find(id);
-		if (!uc)
+		if (IS_ERR_OR_NULL(uc))
 			continue;
 		ut_assertok(uclass_destroy(uc));
 	}
-- 
2.28.0

[PATCH 1/3] dev: Disambiguate errors in uclass_find

[Simon Glass]

Hi Sean,

On Sat, 12 Sep 2020 at 15:46, Sean Anderson <seanga2@gmail.com> wrote:
>
> There are two cases where uclass_find can return an error. The second is
> when the uclass has not yet been init'd. The first is when the driver model
> has not been init'd (or has been only partially init'd) and there is no
> root uclass driver.
>
> If LOG_SYSLOG is enabled and LOG_DEFAULT_LEVEL is set to LOGL_DEBUG or
> higher, log_syslog_emit will try to call net_init before initf_dm has been
> called. This in turn (eventually) calls uclass_get for UCLASS_ETH.
>
> If the second error occurs, uclass_get should call uclass_add to create the
> uclass. If the first error occurs, then uclass_get should not call
> uclass_add (because the uclass list has not been initialized). However, the
> first error is expected when calling uclass_get for UCLASS_ROOT, so we add
> an exception.
>
> There are several alternative approaches to this patch. One option would be
> to duplicate the check against gd->dm_root in uclass_get and not change the
> behavior of uclass_find. I did not choose this approach because I think it
> it is less clear than the patch as-is. However, that is certainly
> subjective, and there is no other technical reason to do it this way.
>
> Another approach would have been to change log_syslog_emit to abort if it
> is called too early. I did not choose this approach because the check in
> uclass_find to see if gd->dm_root exists implies that functions in its
> file are allowed to be called at any time. There is an argument to be made
> that callers should ensure that they don't call certain functions too
> early. I think it is better to code defensively in these circumstances to
> make it easier to discover such errors.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  drivers/core/uclass.c | 16 +++++++++++++++-
>  test/dm/core.c        |  2 +-
>  test/dm/test-main.c   |  2 +-
>  3 files changed, 17 insertions(+), 3 deletions(-)

I'm not sure you can do this. You need to call dm_init() to get DM
running properly.

So in this case I think we need to arrange for the log output to not
happen, before DM is inited.

Regards,
Simon

[PATCH 1/3] dev: Disambiguate errors in uclass_find

[Sean Anderson]

On 9/16/20 9:09 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Sat, 12 Sep 2020 at 15:46, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> There are two cases where uclass_find can return an error. The second is
>> when the uclass has not yet been init'd. The first is when the driver model
>> has not been init'd (or has been only partially init'd) and there is no
>> root uclass driver.
>>
>> If LOG_SYSLOG is enabled and LOG_DEFAULT_LEVEL is set to LOGL_DEBUG or
>> higher, log_syslog_emit will try to call net_init before initf_dm has been
>> called. This in turn (eventually) calls uclass_get for UCLASS_ETH.
>>
>> If the second error occurs, uclass_get should call uclass_add to create the
>> uclass. If the first error occurs, then uclass_get should not call
>> uclass_add (because the uclass list has not been initialized). However, the
>> first error is expected when calling uclass_get for UCLASS_ROOT, so we add
>> an exception.
>>
>> There are several alternative approaches to this patch. One option would be
>> to duplicate the check against gd->dm_root in uclass_get and not change the
>> behavior of uclass_find. I did not choose this approach because I think it
>> it is less clear than the patch as-is. However, that is certainly
>> subjective, and there is no other technical reason to do it this way.
>>
>> Another approach would have been to change log_syslog_emit to abort if it
>> is called too early. I did not choose this approach because the check in
>> uclass_find to see if gd->dm_root exists implies that functions in its
>> file are allowed to be called at any time. There is an argument to be made
>> that callers should ensure that they don't call certain functions too
>> early. I think it is better to code defensively in these circumstances to
>> make it easier to discover such errors.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>  drivers/core/uclass.c | 16 +++++++++++++++-
>>  test/dm/core.c        |  2 +-
>>  test/dm/test-main.c   |  2 +-
>>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> I'm not sure you can do this. You need to call dm_init() to get DM
> running properly.
> 
> So in this case I think we need to arrange for the log output to not
> happen, before DM is inited.

Ok, so do you think the second approach is better? E.g. adding

if (!gd || !(gd->flags & GD_FLG_DEVINIT))
	return 0;

to the beginning of log_syslog_emit?

--Sean

[PATCH 1/3] dev: Disambiguate errors in uclass_find

[Simon Glass]

Hi Sean,

On Wed, 16 Sep 2020 at 19:44, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 9/16/20 9:09 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Sat, 12 Sep 2020 at 15:46, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> There are two cases where uclass_find can return an error. The second is
> >> when the uclass has not yet been init'd. The first is when the driver model
> >> has not been init'd (or has been only partially init'd) and there is no
> >> root uclass driver.
> >>
> >> If LOG_SYSLOG is enabled and LOG_DEFAULT_LEVEL is set to LOGL_DEBUG or
> >> higher, log_syslog_emit will try to call net_init before initf_dm has been
> >> called. This in turn (eventually) calls uclass_get for UCLASS_ETH.
> >>
> >> If the second error occurs, uclass_get should call uclass_add to create the
> >> uclass. If the first error occurs, then uclass_get should not call
> >> uclass_add (because the uclass list has not been initialized). However, the
> >> first error is expected when calling uclass_get for UCLASS_ROOT, so we add
> >> an exception.
> >>
> >> There are several alternative approaches to this patch. One option would be
> >> to duplicate the check against gd->dm_root in uclass_get and not change the
> >> behavior of uclass_find. I did not choose this approach because I think it
> >> it is less clear than the patch as-is. However, that is certainly
> >> subjective, and there is no other technical reason to do it this way.
> >>
> >> Another approach would have been to change log_syslog_emit to abort if it
> >> is called too early. I did not choose this approach because the check in
> >> uclass_find to see if gd->dm_root exists implies that functions in its
> >> file are allowed to be called at any time. There is an argument to be made
> >> that callers should ensure that they don't call certain functions too
> >> early. I think it is better to code defensively in these circumstances to
> >> make it easier to discover such errors.
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> ---
> >>
> >>  drivers/core/uclass.c | 16 +++++++++++++++-
> >>  test/dm/core.c        |  2 +-
> >>  test/dm/test-main.c   |  2 +-
> >>  3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > I'm not sure you can do this. You need to call dm_init() to get DM
> > running properly.
> >
> > So in this case I think we need to arrange for the log output to not
> > happen, before DM is inited.
>
> Ok, so do you think the second approach is better? E.g. adding
>
> if (!gd || !(gd->flags & GD_FLG_DEVINIT))
>         return 0;
>
> to the beginning of log_syslog_emit?

Yes. But how about we have a function in dm.h or similar that tells if
you that driver model is inited? It could be set quite early in
dm_init().

Regards,
Simon

[PATCH 2/3] net: Expose some errors generated in net_init

[Sean Anderson]

net_init does not always succeed, and there is no existing mechanism to
discover errors. This patch allows callers of net_init (such as net_init)
to handle errors. The root issue is that eth_get_dev can fail, but
net_init_loop doesn't expose that. The ideal way to fix eth_get_dev would
be to return an error with ERR_PTR, but there are a lot of callers, and all
of them just check if it's NULL. Another approach would be to change the
signature to something like

int eth_get_dev(struct udevice **pdev)

but that would require rewriting all of the many callers.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 include/net.h    |  2 +-
 net/eth-uclass.c |  3 +++
 net/net.c        | 15 +++++++++++----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/net.h b/include/net.h
index 1bf9867f8c..b80b13ae7c 100644
--- a/include/net.h
+++ b/include/net.h
@@ -593,7 +593,7 @@ extern int net_ntp_time_offset;			/* offset time from UTC */
 #endif
 
 /* Initialize the network adapter */
-void net_init(void);
+int net_init(void);
 int net_loop(enum proto_t);
 
 /* Load failed.	 Start again. */
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 0d9b75a9a2..1f59d24f54 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -75,6 +75,9 @@ struct udevice *eth_get_dev(void)
 	struct eth_uclass_priv *uc_priv;
 
 	uc_priv = eth_get_uclass_priv();
+	if (!uc_priv)
+		return NULL;
+
 	if (!uc_priv->current)
 		eth_errno = uclass_first_device(UCLASS_ETH,
 				    &uc_priv->current);
diff --git a/net/net.c b/net/net.c
index 28d9eebf9d..3afd0d6fb0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -353,12 +353,19 @@ void net_auto_load(void)
 	tftp_start(TFTPGET);
 }
 
-static void net_init_loop(void)
+static int net_init_loop(void)
 {
 	if (eth_get_dev())
 		memcpy(net_ethaddr, eth_get_ethaddr(), 6);
+	else
+		/*
+		 * Not ideal, but there's no way to get the actual error, and I
+		 * don't feel like fixing all the users of eth_get_dev to deal
+		 * with errors.
+		 */
+		return -ENONET;
 
-	return;
+	return 0;
 }
 
 static void net_clear_handlers(void)
@@ -373,7 +380,7 @@ static void net_cleanup_loop(void)
 	net_clear_handlers();
 }
 
-void net_init(void)
+int net_init(void)
 {
 	static int first_call = 1;
 
@@ -396,7 +403,7 @@ void net_init(void)
 		first_call = 0;
 	}
 
-	net_init_loop();
+	return net_init_loop();
 }
 
 /**********************************************************************/
-- 
2.28.0

[PATCH 2/3] net: Expose some errors generated in net_init

[Simon Glass]

On Sat, 12 Sep 2020 at 15:46, Sean Anderson <seanga2@gmail.com> wrote:
>
> net_init does not always succeed, and there is no existing mechanism to
> discover errors. This patch allows callers of net_init (such as net_init)
> to handle errors. The root issue is that eth_get_dev can fail, but
> net_init_loop doesn't expose that. The ideal way to fix eth_get_dev would
> be to return an error with ERR_PTR, but there are a lot of callers, and all
> of them just check if it's NULL. Another approach would be to change the
> signature to something like
>
> int eth_get_dev(struct udevice **pdev)
>
> but that would require rewriting all of the many callers.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  include/net.h    |  2 +-
>  net/eth-uclass.c |  3 +++
>  net/net.c        | 15 +++++++++++----
>  3 files changed, 15 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/3] net: Expose some errors generated in net_init

[Tom Rini]

On Sat, Sep 12, 2020 at 05:45:43PM -0400, Sean Anderson wrote:

> net_init does not always succeed, and there is no existing mechanism to
> discover errors. This patch allows callers of net_init (such as net_init)
> to handle errors. The root issue is that eth_get_dev can fail, but
> net_init_loop doesn't expose that. The ideal way to fix eth_get_dev would
> be to return an error with ERR_PTR, but there are a lot of callers, and all
> of them just check if it's NULL. Another approach would be to change the
> signature to something like
> 
> int eth_get_dev(struct udevice **pdev)
> 
> but that would require rewriting all of the many callers.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201011/6edd7af7/attachment.sig>

[PATCH 3/3] log: syslog: Handle errors in net_init

[Sean Anderson]

Since the previous patch, net_init now exposes some errors, so check for
them.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log_syslog.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/log_syslog.c b/common/log_syslog.c
index 149ff5af31..aaa0fcfa48 100644
--- a/common/log_syslog.c
+++ b/common/log_syslog.c
@@ -46,7 +46,9 @@ static int log_syslog_emit(struct log_device *ldev, struct log_rec *rec)
 	processing_msg = 1;
 
 	/* Setup packet buffers */
-	net_init();
+	ret = net_init();
+	if (ret)
+		return ret;
 	/* Disable hardware and put it into the reset state */
 	eth_halt();
 	/* Set current device according to environment variables */
-- 
2.28.0

[PATCH 3/3] log: syslog: Handle errors in net_init

[Simon Glass]

On Sat, 12 Sep 2020 at 15:46, Sean Anderson <seanga2@gmail.com> wrote:
>
> Since the previous patch, net_init now exposes some errors, so check for
> them.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  common/log_syslog.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/3] log: syslog: Handle errors in net_init

[Tom Rini]

On Sat, Sep 12, 2020 at 05:45:44PM -0400, Sean Anderson wrote:

> Since the previous patch, net_init now exposes some errors, so check for
> them.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201011/44561ca4/attachment.sig>

[PATCH 0/3] log: Fix segfault in sandbox when LOG_LEVEL_DEFAULT >= LOGL_DEBUG

[Sean Anderson]

On 9/12/20 5:45 PM, Sean Anderson wrote:
> Since the syslog feature has been introduced, sandbox no longer boots when
> LOG_LEVEL_DEFAULT prints cdebug messages. This is because it tries to call
> net_init before initf_dm.
> 
> 
> Sean Anderson (3):
>   dev: Disambiguate errors in uclass_find
>   net: Expose some errors generated in net_init
>   log: syslog: Handle errors in net_init
> 
>  common/log_syslog.c   |  4 +++-
>  drivers/core/uclass.c | 16 +++++++++++++++-
>  include/net.h         |  2 +-
>  net/eth-uclass.c      |  3 +++
>  net/net.c             | 15 +++++++++++----
>  test/dm/core.c        |  2 +-
>  test/dm/test-main.c   |  2 +-
>  7 files changed, 35 insertions(+), 9 deletions(-)
> 

This causes CI to fail in an interesting way [1]. Running `ut log` on a
fresh boot will have no failures. However, if `ut log` is run after `ut
dm`, then the tests will fail. From further investigation, it appears
that sb_log_tx_handler is not called at all when the tests fail.

I don't know whether this is a "real" test failure or a failure of the
unit testing framework. `ut dm` doesn't appear to be idempotent; every
time I run it on the same boot I get additional errors. What's stranger
is that running `ut all` has no failures, but running `ut dm` has 2
(both for usb_flash).

--Sean

[1] https://dev.azure.com/seanga2/u-boot/_build/results?buildId=30&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=455