[PATCH] part: return -ENOSYS when get_info not valid.

[PATCH] part: return -ENOSYS when get_info not valid.

[zhaohui.shi]

From: schspa <schspa@gmail.com>

In some case, get_info() interface can be NULL, add this check to stop
from crash.

Signed-off-by: schspa <schspa@gmail.com>
---
 disk/part.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index a6a8f7052b..7af3240ec7 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct blk_desc *dev_desc, const char *name,
 	part_drv = part_driver_lookup_type(dev_desc);
 	if (!part_drv)
 		return -1;
+
+	if (!part_drv->get_info) {
+		PRINTF("## Driver %s does not have the get_info() method\n",
+		       part_drv->name);
+		return -ENOSYS;
+	}
+
 	for (i = 1; i < part_drv->max_entries; i++) {
 		ret = part_drv->get_info(dev_desc, i, info);
 		if (ret != 0) {
-- 
2.33.1

Re: [PATCH] part: return -ENOSYS when get_info not valid.

[Heinrich Schuchardt]

On 10/19/21 15:35, zhaohui.shi wrote:
> From: schspa <schspa@gmail.com>
>
> In some case, get_info() interface can be NULL, add this check to stop
> from crash.

Thank you for reviewing the partition driver code.

There seems to be no driver missing a get_info implementation. Where did
you run into a problem?

Why should we only check .get_info and not .test and not .print?

>
> Signed-off-by: schspa <schspa@gmail.com>

Please, provide a name.

> ---
>   disk/part.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/disk/part.c b/disk/part.c
> index a6a8f7052b..7af3240ec7 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct blk_desc *dev_desc, const char *name,
>   	part_drv = part_driver_lookup_type(dev_desc);
>   	if (!part_drv)
>   		return -1;
> +
> +	if (!part_drv->get_info) {
> +		PRINTF("## Driver %s does not have the get_info() method\n",
> +		       part_drv->name);

Please, use log_debug() to avoid noise on the console on every boot.

Best regards

Heinrich

> +		return -ENOSYS;
> +	}
> +
>   	for (i = 1; i < part_drv->max_entries; i++) {
>   		ret = part_drv->get_info(dev_desc, i, info);
>   		if (ret != 0) {
>

Re: [PATCH] part: return -ENOSYS when get_info not valid.

[schspa]

在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道：
> On 10/19/21 15:35, zhaohui.shi wrote:
> > From: schspa <schspa@gmail.com>
> > 
> > In some case, get_info() interface can be NULL, add this check to
> > stop
> > from crash.
> 
> Thank you for reviewing the partition driver code.
> 
> There seems to be no driver missing a get_info implementation. Where
> did
> you run into a problem?
> 
Yes, I do run into a problem, In my spl build, CONFIG_SPL_FS_EXT4,
CONFIG_SPL_FS_FAT, CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION are all
not enabled. In this case, get_info implementation is NULL. see
'part_get_info_ptr' and 'part_print_ptr' and part_efi.c for detail.

> Why should we only check .get_info and not .test and not .print?
> 

All part type driver have .test implementation, it can't be NULL,
and .print have NULL pointer judgement already.

> > 
> > Signed-off-by: schspa <schspa@gmail.com>
> 
> Please, provide a name.
> 
> > ---
> >   disk/part.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/disk/part.c b/disk/part.c
> > index a6a8f7052b..7af3240ec7 100644
> > --- a/disk/part.c
> > +++ b/disk/part.c
> > @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct blk_desc
> > *dev_desc, const char *name,
> >         part_drv = part_driver_lookup_type(dev_desc);
> >         if (!part_drv)
> >                 return -1;
> > +
> > +       if (!part_drv->get_info) {
> > +               PRINTF("## Driver %s does not have the get_info()
> > method\n",
> > +                      part_drv->name);
> 
> Please, use log_debug() to avoid noise on the console on every boot.
> 

I think it's OK to use PRINTF, because of this BUG occurs only when
user give a bad part configuration, and this error message can prompt
the user that a configuration error has occurred.
Besides, 'part_get_info' use PRINTF for this null pointer protection
too.

> Best regards
> 
> Heinrich
> 
> > +               return -ENOSYS;
> > +       }
> > +
> >         for (i = 1; i < part_drv->max_entries; i++) {
> >                 ret = part_drv->get_info(dev_desc, i, info);
> >                 if (ret != 0) {
> > 

-- 
Best regards

schspa <schspa@gmail.com>

Re: [PATCH] part: return -ENOSYS when get_info not valid.

[Heinrich Schuchardt]

On 10/20/21 04:37, schspa wrote:
> 在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道：
>> On 10/19/21 15:35, zhaohui.shi wrote:
>>> From: schspa <schspa@gmail.com>
>>>
>>> In some case, get_info() interface can be NULL, add this check to
>>> stop
>>> from crash.
>>
>> Thank you for reviewing the partition driver code.
>>
>> There seems to be no driver missing a get_info implementation. Where
>> did
>> you run into a problem?
>>
> Yes, I do run into a problem, In my spl build, CONFIG_SPL_FS_EXT4,
> CONFIG_SPL_FS_FAT, CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION are all
> not enabled. In this case, get_info implementation is NULL. see
> 'part_get_info_ptr' and 'part_print_ptr' and part_efi.c for detail.
>
>> Why should we only check .get_info and not .test and not .print?
>>
>
> All part type driver have .test implementation, it can't be NULL,
> and .print have NULL pointer judgement already.
>
>>>
>>> Signed-off-by: schspa <schspa@gmail.com>
>>
>> Please, provide a name.
>>
>>> ---
>>>    disk/part.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/disk/part.c b/disk/part.c
>>> index a6a8f7052b..7af3240ec7 100644
>>> --- a/disk/part.c
>>> +++ b/disk/part.c
>>> @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct blk_desc
>>> *dev_desc, const char *name,
>>>          part_drv = part_driver_lookup_type(dev_desc);
>>>          if (!part_drv)
>>>                  return -1;
>>> +
>>> +       if (!part_drv->get_info) {
>>> +               PRINTF("## Driver %s does not have the get_info()
>>> method\n",
>>> +                      part_drv->name);
>>
>> Please, use log_debug() to avoid noise on the console on every boot.
>>
>
> I think it's OK to use PRINTF, because of this BUG occurs only when
> user give a bad part configuration, and this error message can prompt
> the user that a configuration error has occurred.
> Besides, 'part_get_info' use PRINTF for this null pointer protection
> too.

Above you write that part_drv->get_info = NULL is part of your
configuration. So it will always be printed. The size of the SPL is very
critical on many boards. We should avoid anything that increases it.

Why is part_get_info_by_name_type() being called in your configuration
which does not provide a partition driver in SPL? Are there some
dependencies missing in the Kconfig files?

Best regards

Heinrich

>
>> Best regards
>>
>> Heinrich
>>
>>> +               return -ENOSYS;
>>> +       }
>>> +
>>>          for (i = 1; i < part_drv->max_entries; i++) {
>>>                  ret = part_drv->get_info(dev_desc, i, info);
>>>                  if (ret != 0) {
>>>
>

Re: [PATCH] part: return -ENOSYS when get_info not valid.

[schspa]

在 2021-10-20星期三的 06:44 +0200，Heinrich Schuchardt写道：
> 
> 
> On 10/20/21 04:37, schspa wrote:
> > 在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道：
> > > On 10/19/21 15:35, zhaohui.shi wrote:
> > > > From: schspa <schspa@gmail.com>
> > > > 
> > > > In some case, get_info() interface can be NULL, add this check to
> > > > stop
> > > > from crash.
> > > 
> > > Thank you for reviewing the partition driver code.
> > > 
> > > There seems to be no driver missing a get_info implementation.
> > > Where
> > > did
> > > you run into a problem?
> > > 
> > Yes, I do run into a problem, In my spl build, CONFIG_SPL_FS_EXT4,
> > CONFIG_SPL_FS_FAT, CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION are all
> > not enabled. In this case, get_info implementation is NULL. see
> > 'part_get_info_ptr' and 'part_print_ptr' and part_efi.c for detail.
> > 
> > > Why should we only check .get_info and not .test and not .print?
> > > 
> > 
> > All part type driver have .test implementation, it can't be NULL,
> > and .print have NULL pointer judgement already.
> > 
> > > > 
> > > > Signed-off-by: schspa <schspa@gmail.com>
> > > 
> > > Please, provide a name.
> > > 
> > > > ---
> > > >    disk/part.c | 7 +++++++
> > > >    1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/disk/part.c b/disk/part.c
> > > > index a6a8f7052b..7af3240ec7 100644
> > > > --- a/disk/part.c
> > > > +++ b/disk/part.c
> > > > @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct
> > > > blk_desc
> > > > *dev_desc, const char *name,
> > > >          part_drv = part_driver_lookup_type(dev_desc);
> > > >          if (!part_drv)
> > > >                  return -1;
> > > > +
> > > > +       if (!part_drv->get_info) {
> > > > +               PRINTF("## Driver %s does not have the get_info()
> > > > method\n",
> > > > +                      part_drv->name);
> > > 
> > > Please, use log_debug() to avoid noise on the console on every
> > > boot.
> > > 
> > 
> > I think it's OK to use PRINTF, because of this BUG occurs only when
> > user give a bad part configuration, and this error message can prompt
> > the user that a configuration error has occurred.
> > Besides, 'part_get_info' use PRINTF for this null pointer protection
> > too.
> 
> Above you write that part_drv->get_info = NULL is part of your
> configuration. So it will always be printed. The size of the SPL is
> very
> critical on many boards. We should avoid anything that increases it.
> 

Considering this problem, I will upload a new patch to use log_debug()
for error message.

> Why is part_get_info_by_name_type() being called in your configuration
> which does not provide a partition driver in SPL? Are there some
> dependencies missing in the Kconfig files?
> 

In my case, I have 
CONFIG_SPL_EFI_PARTITION=y
CONFIG_SPL_LIBDISK_SUPPORT=y
but forget to enable CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION.


Yes, it seems we can have some dependencies in Kconfig like this:
config SPL_LIBDISK_SUPPORT
	bool "Support disk partitions"
	depends on CONFIG_SPL_EXT_SUPPORT || CONFIG_SPL_FAT_SUPPORT ||
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
	help

But there is some case for part_iso or part_mac, which have no this
dependencies (because of it don't use part_get_info_ptr to wrap
part_get_info_iso).

How do you think about this ?  Should we add such dependencies ?
part_iso seems shouldn't have dependencies about ext, fat, mmcsd etc.


> Best regards
> 
> Heinrich
> 
> > 
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > +               return -ENOSYS;
> > > > +       }
> > > > +
> > > >          for (i = 1; i < part_drv->max_entries; i++) {
> > > >                  ret = part_drv->get_info(dev_desc, i, info);
> > > >                  if (ret != 0) {
> > > > 
> > 

-- 
schspa <schspa@gmail.com>

Re: [PATCH] part: return -ENOSYS when get_info not valid.

[Heinrich Schuchardt]

On 10/20/21 07:54, schspa wrote:
> 在 2021-10-20星期三的 06:44 +0200，Heinrich Schuchardt写道：
>>
>>
>> On 10/20/21 04:37, schspa wrote:
>>> 在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道：
>>>> On 10/19/21 15:35, zhaohui.shi wrote:
>>>>> From: schspa <schspa@gmail.com>
>>>>>
>>>>> In some case, get_info() interface can be NULL, add this check to
>>>>> stop
>>>>> from crash.
>>>>
>>>> Thank you for reviewing the partition driver code.
>>>>
>>>> There seems to be no driver missing a get_info implementation.
>>>> Where
>>>> did
>>>> you run into a problem?
>>>>
>>> Yes, I do run into a problem, In my spl build, CONFIG_SPL_FS_EXT4,
>>> CONFIG_SPL_FS_FAT, CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION are all
>>> not enabled. In this case, get_info implementation is NULL. see
>>> 'part_get_info_ptr' and 'part_print_ptr' and part_efi.c for detail.
>>>
>>>> Why should we only check .get_info and not .test and not .print?
>>>>
>>>
>>> All part type driver have .test implementation, it can't be NULL,
>>> and .print have NULL pointer judgement already.
>>>
>>>>>
>>>>> Signed-off-by: schspa <schspa@gmail.com>
>>>>
>>>> Please, provide a name.
>>>>
>>>>> ---
>>>>>     disk/part.c | 7 +++++++
>>>>>     1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/disk/part.c b/disk/part.c
>>>>> index a6a8f7052b..7af3240ec7 100644
>>>>> --- a/disk/part.c
>>>>> +++ b/disk/part.c
>>>>> @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct
>>>>> blk_desc
>>>>> *dev_desc, const char *name,
>>>>>           part_drv = part_driver_lookup_type(dev_desc);
>>>>>           if (!part_drv)
>>>>>                   return -1;
>>>>> +
>>>>> +       if (!part_drv->get_info) {
>>>>> +               PRINTF("## Driver %s does not have the get_info()
>>>>> method\n",
>>>>> +                      part_drv->name);
>>>>
>>>> Please, use log_debug() to avoid noise on the console on every
>>>> boot.
>>>>
>>>
>>> I think it's OK to use PRINTF, because of this BUG occurs only when
>>> user give a bad part configuration, and this error message can prompt
>>> the user that a configuration error has occurred.
>>> Besides, 'part_get_info' use PRINTF for this null pointer protection
>>> too.
>>
>> Above you write that part_drv->get_info = NULL is part of your
>> configuration. So it will always be printed. The size of the SPL is
>> very
>> critical on many boards. We should avoid anything that increases it.
>>
>
> Considering this problem, I will upload a new patch to use log_debug()
> for error message.
>
>> Why is part_get_info_by_name_type() being called in your configuration
>> which does not provide a partition driver in SPL? Are there some
>> dependencies missing in the Kconfig files?
>>
>
> In my case, I have
> CONFIG_SPL_EFI_PARTITION=y
> CONFIG_SPL_LIBDISK_SUPPORT=y
> but forget to enable CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION.
>
>
> Yes, it seems we can have some dependencies in Kconfig like this:
> config SPL_LIBDISK_SUPPORT
> 	bool "Support disk partitions"
> 	depends on CONFIG_SPL_EXT_SUPPORT || CONFIG_SPL_FAT_SUPPORT ||
> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
> 	help
>
> But there is some case for part_iso or part_mac, which have no this
> dependencies (because of it don't use part_get_info_ptr to wrap
> part_get_info_iso).
>
> How do you think about this ?  Should we add such dependencies ?
> part_iso seems shouldn't have dependencies about ext, fat, mmcsd etc.

Ideally it should not be possible to configure something that does not
compile or will crash. Linux checks random configuration in their CI.

The Kconfig change should be in a separate patch.

Best regards

Heinrich

>
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> +               return -ENOSYS;
>>>>> +       }
>>>>> +
>>>>>           for (i = 1; i < part_drv->max_entries; i++) {
>>>>>                   ret = part_drv->get_info(dev_desc, i, info);
>>>>>                   if (ret != 0) {
>>>>>
>>>
>

[PATCH] part: return -ENOSYS when get_info not valid.

[schspa]

In some case, get_info() interface can be NULL, add this check to stop
from crash.

Signed-off-by: schspa <schspa@gmail.com>
---
 disk/part.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index a6a8f7052b..7af3240ec7 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct blk_desc *dev_desc, const char *name,
 	part_drv = part_driver_lookup_type(dev_desc);
 	if (!part_drv)
 		return -1;
+
+	if (!part_drv->get_info) {
+		PRINTF("## Driver %s does not have the get_info() method\n",
+		       part_drv->name);
+		return -ENOSYS;
+	}
+
 	for (i = 1; i < part_drv->max_entries; i++) {
 		ret = part_drv->get_info(dev_desc, i, info);
 		if (ret != 0) {
-- 
2.33.1