[RESEND][PATCH bpf 0/2] Check the remaining info_cnt before repeating btf fields

[RESEND][PATCH bpf 0/2] Check the remaining info_cnt before repeating btf fields

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Hi,

The patch set adds the missed check again info_cnt when flattening the
array of nested struct. The problem was spotted when developing dynptr
key support for hash map. Patch #1 adds the missed check and patch #2
adds three success test cases and one failure test case for the problem.

Comments are always welcome.

Hou Tao (2):
  bpf: Check the remaining info_cnt before repeating btf fields
  selftests/bpf: Add more test case for field flattening

 kernel/bpf/btf.c                              |  6 ++
 .../selftests/bpf/prog_tests/cpumask.c        |  1 +
 .../selftests/bpf/progs/cpumask_failure.c     | 35 +++++++++
 .../selftests/bpf/progs/cpumask_success.c     | 78 ++++++++++++++++++-
 4 files changed, 118 insertions(+), 2 deletions(-)

-- 
2.29.2

[RESEND][PATCH bpf 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

When trying to repeat the btf fields for array of nested struct, it
doesn't check the remaining info_cnt. The following splat will be
reported when the value of ret * nelems is greater than BTF_FIELDS_MAX:

  ------------[ cut here ]------------
  UBSAN: array-index-out-of-bounds in ../kernel/bpf/btf.c:3951:49
  index 12 is out of range for type 'btf_field_info [12]'
  CPU: 6 UID: 0 PID: 411 Comm: test_progs ...... 6.11.0-rc4+ #1
  Tainted: [O]=OOT_MODULE
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ...
  Call Trace:
   <TASK>
   dump_stack_lvl+0x57/0x70
   dump_stack+0x10/0x20
   ubsan_epilogue+0x9/0x40
   __ubsan_handle_out_of_bounds+0x6f/0x80
   ? kallsyms_lookup_name+0x48/0xb0
   btf_parse_fields+0x992/0xce0
   map_create+0x591/0x770
   __sys_bpf+0x229/0x2410
   __x64_sys_bpf+0x1f/0x30
   x64_sys_call+0x199/0x9f0
   do_syscall_64+0x3b/0xc0
   entry_SYSCALL_64_after_hwframe+0x4b/0x53
  RIP: 0033:0x7fea56f2cc5d
  ......
   </TASK>
  ---[ end trace ]---

Fix it by checking the remaining info_cnt before btf field repetition.

Fixes: 64e8ee814819 ("bpf: look into the types of the fields of a struct type recursively.")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/bpf/btf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index a4e4f8d43ecf..9a4a074d26f5 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
 		info[i].off += off;
 
 	if (nelems > 1) {
+		/* The type of struct size or variable size is u32,
+		 * so the multiplication will not overflow.
+		 */
+		if (ret * nelems > info_cnt)
+			return -E2BIG;
+
 		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
 		if (err == 0)
 			ret *= nelems;
-- 
2.29.2

[RESEND][PATCH bpf 2/2] selftests/bpf: Add more test case for field flattening

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Add three success test cases to test the flattening of array of nested
struct. For these three tests, the number of special fields in map is
BTF_FIELDS_MAX, but the array is defined in structs with different
nested level.

Add one failure test case for the flattening as well. In the test case,
the number of special fields in map is BTF_FIELDS_MAX + 1. It will make
btf_parse_fields() in map_create() return -E2BIG, the creation of map
will succeed, but the load of program will fail because the btf_record
is invalid for the map.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/cpumask.c        |  1 +
 .../selftests/bpf/progs/cpumask_failure.c     | 35 +++++++++
 .../selftests/bpf/progs/cpumask_success.c     | 78 ++++++++++++++++++-
 3 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/cpumask.c b/tools/testing/selftests/bpf/prog_tests/cpumask.c
index 2570bd4b0cb2..e58a04654238 100644
--- a/tools/testing/selftests/bpf/prog_tests/cpumask.c
+++ b/tools/testing/selftests/bpf/prog_tests/cpumask.c
@@ -23,6 +23,7 @@ static const char * const cpumask_success_testcases[] = {
 	"test_global_mask_array_l2_rcu",
 	"test_global_mask_nested_rcu",
 	"test_global_mask_nested_deep_rcu",
+	"test_global_mask_nested_deep_array_rcu",
 	"test_cpumask_weight",
 };
 
diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
index a988d2823b52..e9cb93ce9533 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
@@ -10,6 +10,21 @@
 
 char _license[] SEC("license") = "GPL";
 
+struct kptr_nested_array_2 {
+	struct bpf_cpumask __kptr * mask;
+};
+
+struct kptr_nested_array_1 {
+	/* Make btf_parse_fields() in map_create() return -E2BIG */
+	struct kptr_nested_array_2 d_2[BTF_FIELDS_MAX + 1];
+};
+
+struct kptr_nested_array {
+	struct kptr_nested_array_1 d_1;
+};
+
+private(MASK_NESTED) static struct kptr_nested_array global_mask_nested_array;
+
 /* Prototype for all of the program trace events below:
  *
  * TRACE_EVENT(task_newtask,
@@ -187,3 +202,23 @@ int BPF_PROG(test_global_mask_rcu_no_null_check, struct task_struct *task, u64 c
 
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+__failure __msg("has no valid kptr")
+int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *local, *prev;
+
+	local = create_cpumask();
+	if (!local)
+		return 0;
+
+	prev = bpf_kptr_xchg(&global_mask_nested_array.d_1.d_2[BTF_FIELDS_MAX].mask, local);
+	if (prev) {
+		bpf_cpumask_release(prev);
+		err = 3;
+		return 0;
+	}
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index fd8106831c32..c634a34dec51 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -31,11 +31,59 @@ struct kptr_nested_deep {
 	struct kptr_nested_pair ptr_pairs[3];
 };
 
+struct kptr_nested_deep_array_1_2 {
+	int dummy;
+	struct bpf_cpumask __kptr * mask[BTF_FIELDS_MAX];
+};
+
+struct kptr_nested_deep_array_1_1 {
+	int dummy;
+	struct kptr_nested_deep_array_1_2 d_2;
+};
+
+struct kptr_nested_deep_array_1 {
+	long dummy;
+	struct kptr_nested_deep_array_1_1 d_1;
+};
+
+struct kptr_nested_deep_array_2_2 {
+	long dummy[2];
+	struct bpf_cpumask __kptr * mask;
+};
+
+struct kptr_nested_deep_array_2_1 {
+	int dummy;
+	struct kptr_nested_deep_array_2_2 d_2[BTF_FIELDS_MAX];
+};
+
+struct kptr_nested_deep_array_2 {
+	long dummy;
+	struct kptr_nested_deep_array_2_1 d_1;
+};
+
+struct kptr_nested_deep_array_3_2 {
+	long dummy[2];
+	struct bpf_cpumask __kptr * mask;
+};
+
+struct kptr_nested_deep_array_3_1 {
+	int dummy;
+	struct kptr_nested_deep_array_3_2 d_2;
+};
+
+struct kptr_nested_deep_array_3 {
+	long dummy;
+	struct kptr_nested_deep_array_3_1 d_1[BTF_FIELDS_MAX];
+};
+
 private(MASK) static struct bpf_cpumask __kptr * global_mask_array[2];
 private(MASK) static struct bpf_cpumask __kptr * global_mask_array_l2[2][1];
 private(MASK) static struct bpf_cpumask __kptr * global_mask_array_one[1];
 private(MASK) static struct kptr_nested global_mask_nested[2];
 private(MASK_DEEP) static struct kptr_nested_deep global_mask_nested_deep;
+private(MASK_1) static struct kptr_nested_deep_array_1 global_mask_nested_deep_array_1;
+private(MASK_2) static struct kptr_nested_deep_array_2 global_mask_nested_deep_array_2;
+private(MASK_3) static struct kptr_nested_deep_array_3 global_mask_nested_deep_array_3;
 
 static bool is_test_task(void)
 {
@@ -543,12 +591,21 @@ static int _global_mask_array_rcu(struct bpf_cpumask **mask0,
 		goto err_exit;
 	}
 
-	/* [<mask 0>, NULL] */
-	if (!*mask0 || *mask1) {
+	/* [<mask 0>, *] */
+	if (!*mask0) {
 		err = 2;
 		goto err_exit;
 	}
 
+	if (!mask1)
+		goto err_exit;
+
+	/* [*, NULL] */
+	if (*mask1) {
+		err = 3;
+		goto err_exit;
+	}
+
 	local = create_cpumask();
 	if (!local) {
 		err = 9;
@@ -631,6 +688,23 @@ int BPF_PROG(test_global_mask_nested_deep_rcu, struct task_struct *task, u64 clo
 	return 0;
 }
 
+SEC("tp_btf/task_newtask")
+int BPF_PROG(test_global_mask_nested_deep_array_rcu, struct task_struct *task, u64 clone_flags)
+{
+	int i;
+
+	for (i = 0; i < BTF_FIELDS_MAX; i++)
+		_global_mask_array_rcu(&global_mask_nested_deep_array_1.d_1.d_2.mask[i], NULL);
+
+	for (i = 0; i < BTF_FIELDS_MAX; i++)
+		_global_mask_array_rcu(&global_mask_nested_deep_array_2.d_1.d_2[i].mask, NULL);
+
+	for (i = 0; i < BTF_FIELDS_MAX; i++)
+		_global_mask_array_rcu(&global_mask_nested_deep_array_3.d_1[i].d_2.mask, NULL);
+
+	return 0;
+}
+
 SEC("tp_btf/task_newtask")
 int BPF_PROG(test_cpumask_weight, struct task_struct *task, u64 clone_flags)
 {
-- 
2.29.2

Re: [RESEND][PATCH bpf 2/2] selftests/bpf: Add more test case for field flattening

[Eduard Zingerman]

On Wed, 2024-09-11 at 19:05 +0800, Hou Tao wrote:

[...]

> diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> index a988d2823b52..e9cb93ce9533 100644
> --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
> +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> @@ -10,6 +10,21 @@
>  
>  char _license[] SEC("license") = "GPL";
>  
> +struct kptr_nested_array_2 {
> +	struct bpf_cpumask __kptr * mask;
> +};
> +
> +struct kptr_nested_array_1 {
> +	/* Make btf_parse_fields() in map_create() return -E2BIG */
> +	struct kptr_nested_array_2 d_2[BTF_FIELDS_MAX + 1];

Hi Huo,

I think some headers are missing, I see the following error when
compiling this test:

progs/cpumask_failure.c:19:33: error: use of undeclared identifier 'BTF_FIELDS_MAX'; did you mean 'BTF_KIND_MAX'?
   19 |         struct kptr_nested_array_2 d_2[BTF_FIELDS_MAX + 1];
      |                                        ^~~~~~~~~~~~~~
      |                                        BTF_KIND_MAX

[...]

Re: [RESEND][PATCH bpf 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Eduard Zingerman]

On Wed, 2024-09-11 at 19:05 +0800, Hou Tao wrote:


[...]

> ---
>  kernel/bpf/btf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index a4e4f8d43ecf..9a4a074d26f5 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
>  		info[i].off += off;
>  
>  	if (nelems > 1) {
> +		/* The type of struct size or variable size is u32,
> +		 * so the multiplication will not overflow.
> +		 */
> +		if (ret * nelems > info_cnt)
> +			return -E2BIG;
> +
>  		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
>  		if (err == 0)
>  			ret *= nelems;


btf_repeat_fields(struct btf_field_info *info,
                  u32 field_cnt, u32 repeat_cnt, u32 elem_size)

copies field "field_cnt * repeat_cnt" times,
in this case field_cnt == ret, repeat_cnt == nelems - 1,
should the check be "ret * (nelems - 1) > info_cnt"?

I suggest to add info_cnt as a parameter of btf_repeat_fields() and do
this check there. So that the check won't be forgotten again if
btf_repeat_fields() is used elsewhere. Wdyt?

Re: [RESEND][PATCH bpf 2/2] selftests/bpf: Add more test case for field flattening

[Hou Tao]

Hi,

On 9/12/2024 1:07 AM, Eduard Zingerman wrote:
> On Wed, 2024-09-11 at 19:05 +0800, Hou Tao wrote:
>
> [...]
>
>> diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
>> index a988d2823b52..e9cb93ce9533 100644
>> --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
>> +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
>> @@ -10,6 +10,21 @@
>>  
>>  char _license[] SEC("license") = "GPL";
>>  
>> +struct kptr_nested_array_2 {
>> +	struct bpf_cpumask __kptr * mask;
>> +};
>> +
>> +struct kptr_nested_array_1 {
>> +	/* Make btf_parse_fields() in map_create() return -E2BIG */
>> +	struct kptr_nested_array_2 d_2[BTF_FIELDS_MAX + 1];
> Hi Huo,
>
> I think some headers are missing, I see the following error when
> compiling this test:
>
> progs/cpumask_failure.c:19:33: error: use of undeclared identifier 'BTF_FIELDS_MAX'; did you mean 'BTF_KIND_MAX'?
>    19 |         struct kptr_nested_array_2 d_2[BTF_FIELDS_MAX + 1];
>       |                                        ^~~~~~~~~~~~~~
>       |                                        BTF_KIND_MAX
>
> [...]

BTF_FIELDS_MAX should be defined in vmlinux.h. Could you please check
whether or not it is present ? It seems that BPF CI reports the same
problem for  build-x86_64-llvm-17/build-x86_64-llvm-18 [1], but others
build are OK.  Do you know  is there anything special about
build-x86_64-llvm-17/18 ?

Re: [RESEND][PATCH bpf 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Hou Tao]

Hi,

On 9/12/2024 1:37 AM, Eduard Zingerman wrote:
> On Wed, 2024-09-11 at 19:05 +0800, Hou Tao wrote:
>
>
> [...]
>
>> ---
>>  kernel/bpf/btf.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index a4e4f8d43ecf..9a4a074d26f5 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
>>  		info[i].off += off;
>>  
>>  	if (nelems > 1) {
>> +		/* The type of struct size or variable size is u32,
>> +		 * so the multiplication will not overflow.
>> +		 */
>> +		if (ret * nelems > info_cnt)
>> +			return -E2BIG;
>> +
>>  		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
>>  		if (err == 0)
>>  			ret *= nelems;
>
> btf_repeat_fields(struct btf_field_info *info,
>                   u32 field_cnt, u32 repeat_cnt, u32 elem_size)
>
> copies field "field_cnt * repeat_cnt" times,
> in this case field_cnt == ret, repeat_cnt == nelems - 1,
> should the check be "ret * (nelems - 1) > info_cnt"?

No. The number of available btf_field_info is info_cnt,
btf_find_struct_field() has already used ret fields, and there are still
ret * (nelems - 1) fields waiting for repetition, so checking ret *
nelems against info_cnt is correct.
>
> I suggest to add info_cnt as a parameter of btf_repeat_fields() and do
> this check there. So that the check won't be forgotten again if
> btf_repeat_fields() is used elsewhere. Wdyt?
Will do in v2.

Re: [RESEND][PATCH bpf 2/2] selftests/bpf: Add more test case for field flattening

[Eduard Zingerman]

On Thu, 2024-09-12 at 09:05 +0800, Hou Tao wrote:

[...]

> > progs/cpumask_failure.c:19:33: error: use of undeclared identifier 'BTF_FIELDS_MAX'; did you mean 'BTF_KIND_MAX'?
> >    19 |         struct kptr_nested_array_2 d_2[BTF_FIELDS_MAX + 1];
> >       |                                        ^~~~~~~~~~~~~~
> >       |                                        BTF_KIND_MAX
> > 
> > [...]
> 
> BTF_FIELDS_MAX should be defined in vmlinux.h. Could you please check
> whether or not it is present ? It seems that BPF CI reports the same
> problem for  build-x86_64-llvm-17/build-x86_64-llvm-18 [1], but others

(did you mean to specify a ling for [1] ?)

> build are OK.  Do you know  is there anything special about
> build-x86_64-llvm-17/18 ?
> 

Hm, this is interesting. I use LLVM 20.0.0git.
For BTF_FIELDS_MAX to be present in vmlinux.h it first has to be
present in vmlinux DWARF. However, the following output is empty:

  $ llvm-dwarfdump vmlinux | grep BTF_FIELDS_MAX

While picking some other enum literal:

  $ llvm-dwarfdump vmlinux | grep BTF_KIND_INT
                  DW_AT_name	("BTF_KIND_INT")
                  DW_AT_name	("BTF_KIND_INT")
                  ...

Produce output.

Looks like LLVM20 drops info about BTF_FIELDS_MAX for some reason.
I will take a look at compiler internals, but you would need some
workaround for the test, unfortunately.

Re: [RESEND][PATCH bpf 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Eduard Zingerman]

On Thu, 2024-09-12 at 09:20 +0800, Hou Tao wrote:

[...]

> > > @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
> > >  		info[i].off += off;
> > >  
> > >  	if (nelems > 1) {
> > > +		/* The type of struct size or variable size is u32,
> > > +		 * so the multiplication will not overflow.
> > > +		 */
> > > +		if (ret * nelems > info_cnt)
> > > +			return -E2BIG;
> > > +
> > >  		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
> > >  		if (err == 0)
> > >  			ret *= nelems;
> > 
> > btf_repeat_fields(struct btf_field_info *info,
> >                   u32 field_cnt, u32 repeat_cnt, u32 elem_size)
> > 
> > copies field "field_cnt * repeat_cnt" times,
> > in this case field_cnt == ret, repeat_cnt == nelems - 1,
> > should the check be "ret * (nelems - 1) > info_cnt"?
> 
> No. The number of available btf_field_info is info_cnt,
> btf_find_struct_field() has already used ret fields, and there are still
> ret * (nelems - 1) fields waiting for repetition, so checking ret *
> nelems against info_cnt is correct.

Please bear with me. Here is btf_repeat_fields():

    static int btf_repeat_fields(struct btf_field_info *info,
                     u32 field_cnt, u32 repeat_cnt, u32 elem_size)
    {
        u32 i, j;
        u32 cur;
        ...
        cur = field_cnt;
        for (i = 0; i < repeat_cnt; i++) {
            ...
            for (j = 0; j < field_cnt; j++)
                info[cur++].off += (i + 1) * elem_size;
        }
        ...
    }

The range for 'cur' is [field_cnt .. field_cnt * repeat_cnt].
Meaning that at-least 'field_cnt * repeat_cnt' entries are necessary
in the 'info' array.
Given parameters passed to the function, this is 'ret * (nelems - 1)'.
What do I miss?

Re: [RESEND][PATCH bpf 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Eduard Zingerman]

On Wed, 2024-09-11 at 20:03 -0700, Eduard Zingerman wrote:

[...]

> Please bear with me. Here is btf_repeat_fields():
> 
>     static int btf_repeat_fields(struct btf_field_info *info,
>                      u32 field_cnt, u32 repeat_cnt, u32 elem_size)
>     {
>         u32 i, j;
>         u32 cur;
>         ...
>         cur = field_cnt;
>         for (i = 0; i < repeat_cnt; i++) {
>             ...
>             for (j = 0; j < field_cnt; j++)
>                 info[cur++].off += (i + 1) * elem_size;
>         }
>         ...
>     }
> 
> The range for 'cur' is [field_cnt .. field_cnt * repeat_cnt].
> Meaning that at-least 'field_cnt * repeat_cnt' entries are necessary
> in the 'info' array.

Ok, I'm wrong.
The range for 'cur' is [field_cnt .. field_cnt * (repeat_cnt + 1)].
So with parameters passed maximal value of 'cur' is 'ret * nelems' indeed.
Sorry for the noise.

Re: [RESEND][PATCH bpf 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Eduard Zingerman]

On Thu, 2024-09-12 at 21:13 +0200, Thinker Li wrote:

[...]

> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index a4e4f8d43ecf..9a4a074d26f5 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
> > >                info[i].off += off;
> > >   
> > >        if (nelems > 1) {
> > > +             /* The type of struct size or variable size is u32,
> > > +              * so the multiplication will not overflow.
> > > +              */
> > > +             if (ret * nelems > info_cnt)
> > > +                     return -E2BIG;
> > > +
> > >                err = btf_repeat_fields(info, ret, nelems - 1, t->size);
> > >                if (err == 0)
> > >                        ret *= nelems;
> > 
> > 
> > btf_repeat_fields(struct btf_field_info *info,
> >                   u32 field_cnt, u32 repeat_cnt, u32 elem_size)
> > 
> > copies field "field_cnt * repeat_cnt" times,
> > in this case field_cnt == ret, repeat_cnt == nelems - 1,
> > should the check be "ret * (nelems - 1) > info_cnt"?
> > 
> > I suggest to add info_cnt as a parameter of btf_repeat_fields() and do
> > this check there. So that the check won't be forgotten again if
> > btf_repeat_fields() is used elsewhere. Wdyt?
> > 
> 
> Should not this check be moved before the earlier for-loop?

Shouldn't the check for 'ret <= 0' be enough to make sure the for-loop
is fine?