diff mbox series

[1/3] dev: Disambiguate errors in uclass_find

Message ID 20200912214544.362594-2-seanga2@gmail.com
State Changes Requested
Delegated to: Simon Glass
Headers show
Series log: Fix segfault in sandbox when LOG_LEVEL_DEFAULT >= LOGL_DEBUG | expand

Commit Message

Sean Anderson Sept. 12, 2020, 9:45 p.m. UTC
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(-)

Comments

Simon Glass Sept. 17, 2020, 1:09 a.m. UTC | #1
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
Sean Anderson Sept. 17, 2020, 1:44 a.m. UTC | #2
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
Simon Glass Sept. 17, 2020, 3:44 a.m. UTC | #3
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
diff mbox series

Patch

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@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));
 	}