| Message ID | 20250112034213.13153-2-semen.protsenko@linaro.org |
|---|---|
| State | Accepted |
| Commit | 11319e0e2bbf7b3892658d45017cd80c41ad961b |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | bootstd: Fix efi_mgr usage in bootmeths env var | expand |
On 1/12/25 04:42, Sam Protsenko wrote: > Free memory allocated for 'order' (array of bootmeths) on error paths in > bootmeth_setup_iter_order() function. > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > boot/bootmeth-uclass.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c > index 5b5fea39b3b3..ff36da78d5a1 100644 > --- a/boot/bootmeth-uclass.c > +++ b/boot/bootmeth-uclass.c > @@ -133,8 +133,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) > * We don't support skipping global bootmeths. Instead, the user > * should omit them from the ordering > */ > - if (!include_global) > - return log_msg_ret("glob", -EPERM); > + if (!include_global) { > + ret = log_msg_ret("glob", -EPERM); > + goto err_order; > + } > memcpy(order, std->bootmeth_order, > count * sizeof(struct bootmeth *)); > > @@ -188,8 +190,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) > } > count = upto; > } > - if (!count) > - return log_msg_ret("count2", -ENOENT); > + if (!count) { > + ret = log_msg_ret("count2", -ENOENT); > + goto err_order; > + } > > if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global && > iter->first_glob_method != -1 && iter->first_glob_method != count) { > @@ -200,6 +204,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) > iter->num_methods = count; > > return 0; > + > +err_order: > + free(order); > + return ret; > } > > int bootmeth_set_order(const char *order_str) bootmeth_setup_iter_order() is called when the `boot scan` command is executed. The command can be executed multiple times, shouldn't we free iter->method_order before reassigning it? Hopefully the field is NULL if not initialized. Best regards Heinrich
On Sat, 11 Jan 2025 at 20:42, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Free memory allocated for 'order' (array of bootmeths) on error paths in > bootmeth_setup_iter_order() function. > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > boot/bootmeth-uclass.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> How about adding a test to check there is no leak? You can see test/lib/abuf.c for similar tests.
On Mon, Jan 13, 2025 at 1:37 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/12/25 04:42, Sam Protsenko wrote: > > Free memory allocated for 'order' (array of bootmeths) on error paths in > > bootmeth_setup_iter_order() function. > > > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > boot/bootmeth-uclass.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c > > index 5b5fea39b3b3..ff36da78d5a1 100644 > > --- a/boot/bootmeth-uclass.c > > +++ b/boot/bootmeth-uclass.c > > @@ -133,8 +133,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) > > * We don't support skipping global bootmeths. Instead, the user > > * should omit them from the ordering > > */ > > - if (!include_global) > > - return log_msg_ret("glob", -EPERM); > > + if (!include_global) { > > + ret = log_msg_ret("glob", -EPERM); > > + goto err_order; > > + } > > memcpy(order, std->bootmeth_order, > > count * sizeof(struct bootmeth *)); > > > > @@ -188,8 +190,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) > > } > > count = upto; > > } > > - if (!count) > > - return log_msg_ret("count2", -ENOENT); > > + if (!count) { > > + ret = log_msg_ret("count2", -ENOENT); > > + goto err_order; > > + } > > > > if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global && > > iter->first_glob_method != -1 && iter->first_glob_method != count) { > > @@ -200,6 +204,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) > > iter->num_methods = count; > > > > return 0; > > + > > +err_order: > > + free(order); > > + return ret; > > } > > > > int bootmeth_set_order(const char *order_str) > > bootmeth_setup_iter_order() is called when the `boot scan` command is > executed. The command can be executed multiple times, shouldn't we free > iter->method_order before reassigning it? Hopefully the field is NULL if > not initialized. > I think it's already taken care of in do_bootflow_scan(), when it's running bootflow_iter_uninit() in the end of the loop? The relevant call chain looks like this: do_bootflow_scan() bootflow_scan_first() bootflow_iter_init() memset(iter, 0) bootmeth_setup_iter_order() order = calloc() iter->method_order = order bootflow_scan_next() bootflow_iter_uninit() free(iter->method_order) So bootmeth_setup_iter_order() is only called once for each 'bootflow scan' execution, and it's always called with iter->method_order freed. > Best regards > > Heinrich >
On Tue, Jan 14, 2025 at 7:14 AM Simon Glass <sjg@chromium.org> wrote: > > On Sat, 11 Jan 2025 at 20:42, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > Free memory allocated for 'order' (array of bootmeths) on error paths in > > bootmeth_setup_iter_order() function. > > > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > boot/bootmeth-uclass.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > How about adding a test to check there is no leak? You can see > test/lib/abuf.c for similar tests. That might be a good idea, and I already have the test code handy. But frankly I wasn't able to successfully run the current bootstd test in sandbox, it fails before it can get to memleak check I added. Firstly, when I do: $ make sandbox_defconfig $ make -j32 $ ./u-boot -T -c "ut bootstd" it says: MMC: Can't map file 'mmc1.img': Invalid argument and I can see only empty (zero size) mmc1.img in my U-Boot source code root dir. Then I was able to trigger its creation (I guess) by running Python suite somehow, so it generated 20 MiB mmc1.img. But even with that added, I hit this error: test/boot/bootdev.c:218, bootdev_test_order(): 0 == bootflow_scan_first(((void *)0), ((void *)0), &iter, 0, &bflow): Expected 0x0 (0), got 0xffffffed (-19) I've read corresponding doc [1] ("Tests" section), but still don't understand how can I successfully run the bootstd test locally. If you can help me out, I can send the memleak test you mentioned in v2. Thanks! [1] doc/develop/bootstd/overview.rst
Hi Sam, On Fri, 7 Feb 2025 at 18:11, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Tue, Jan 14, 2025 at 7:14 AM Simon Glass <sjg@chromium.org> wrote: > > > > On Sat, 11 Jan 2025 at 20:42, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > Free memory allocated for 'order' (array of bootmeths) on error paths in > > > bootmeth_setup_iter_order() function. > > > > > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > > > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > --- > > > boot/bootmeth-uclass.c | 16 ++++++++++++---- > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > How about adding a test to check there is no leak? You can see > > test/lib/abuf.c for similar tests. > > That might be a good idea, and I already have the test code handy. But > frankly I wasn't able to successfully run the current bootstd test in > sandbox, it fails before it can get to memleak check I added. Firstly, > when I do: > > $ make sandbox_defconfig > $ make -j32 > $ ./u-boot -T -c "ut bootstd" > > it says: > > MMC: Can't map file 'mmc1.img': Invalid argument > > and I can see only empty (zero size) mmc1.img in my U-Boot source code > root dir. Then I was able to trigger its creation (I guess) by running > Python suite somehow, so it generated 20 MiB mmc1.img. But even with > that added, I hit this error: > > test/boot/bootdev.c:218, bootdev_test_order(): > 0 == bootflow_scan_first(((void *)0), ((void *)0), &iter, 0, &bflow): > Expected 0x0 (0), got 0xffffffed (-19) > > I've read corresponding doc [1] ("Tests" section), but still don't > understand how can I successfully run the bootstd test locally. If you > can help me out, I can send the memleak test you mentioned in v2. You can run the bootstd tests with something like: test/py/test.py -B sandbox --build-dir /tmp/b/sandbox -k bootstd although it sounds like you have already figured that out. After you do it once (to create the images) you can run the C tests directly as you did above, but yes, sadly, some of the tests are affected by others (with bootstd). You could take a look if you like. Regards, Simon > > Thanks! > > [1] doc/develop/bootstd/overview.rst
On Fri, Feb 7, 2025 at 7:26 PM Simon Glass <sjg@chromium.org> wrote: > > Hi Sam, > > On Fri, 7 Feb 2025 at 18:11, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > On Tue, Jan 14, 2025 at 7:14 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > On Sat, 11 Jan 2025 at 20:42, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > > > Free memory allocated for 'order' (array of bootmeths) on error paths in > > > > bootmeth_setup_iter_order() function. > > > > > > > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > > > > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > --- > > > > boot/bootmeth-uclass.c | 16 ++++++++++++---- > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > How about adding a test to check there is no leak? You can see > > > test/lib/abuf.c for similar tests. > > > > That might be a good idea, and I already have the test code handy. But > > frankly I wasn't able to successfully run the current bootstd test in > > sandbox, it fails before it can get to memleak check I added. Firstly, > > when I do: > > > > $ make sandbox_defconfig > > $ make -j32 > > $ ./u-boot -T -c "ut bootstd" > > > > it says: > > > > MMC: Can't map file 'mmc1.img': Invalid argument > > > > and I can see only empty (zero size) mmc1.img in my U-Boot source code > > root dir. Then I was able to trigger its creation (I guess) by running > > Python suite somehow, so it generated 20 MiB mmc1.img. But even with > > that added, I hit this error: > > > > test/boot/bootdev.c:218, bootdev_test_order(): > > 0 == bootflow_scan_first(((void *)0), ((void *)0), &iter, 0, &bflow): > > Expected 0x0 (0), got 0xffffffed (-19) > > > > I've read corresponding doc [1] ("Tests" section), but still don't > > understand how can I successfully run the bootstd test locally. If you > > can help me out, I can send the memleak test you mentioned in v2. > > You can run the bootstd tests with something like: > > test/py/test.py -B sandbox --build-dir /tmp/b/sandbox -k bootstd > > although it sounds like you have already figured that out. After you > do it once (to create the images) you can run the C tests directly as > you did above, but yes, sadly, some of the tests are affected by > others (with bootstd). You could take a look if you like. > Is it possible to just apply this patch without a test? It's clearly fixing a memleak. Not that I'm lazy or anything, I came up with a test like below, but I don't want to send something that is quite hard to test. If you think this test is really essential, I can send it once bootstd tests are fixed and can be run in one command without any extra non-documented work. 8<---------------------------------------------------------------------->8 diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c index 9af947868707..f4746ee4fb61 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -194,6 +194,7 @@ static int bootdev_test_order(struct unit_test_state *uts) { struct bootflow_iter iter; struct bootflow bflow; + ulong start; test_set_skip_delays(true); @@ -201,6 +202,8 @@ static int bootdev_test_order(struct unit_test_state *uts) bootstd_reset_usb(); ut_assertok(run_command("usb start", 0)); + start = ut_check_free(); + /* * First try the order set by the bootdev-order property * Like all sandbox unit tests this relies on the devicetree setting up @@ -277,6 +280,9 @@ static int bootdev_test_order(struct unit_test_state *uts) iter.dev_used[3]->name); bootflow_iter_uninit(&iter); + /* Check for memory leaks */ + ut_assertok(ut_check_delta(start)); + return 0; } BOOTSTD_TEST(bootdev_test_order, UTF_DM | UTF_SCAN_FDT); 8<---------------------------------------------------------------------->8 > Regards, > Simon > > > > > Thanks! > > > > [1] doc/develop/bootstd/overview.rst
On Thu, Jun 19, 2025 at 05:04:23PM -0500, Sam Protsenko wrote: > On Fri, Feb 7, 2025 at 7:26 PM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sam, > > > > On Fri, 7 Feb 2025 at 18:11, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > On Tue, Jan 14, 2025 at 7:14 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > On Sat, 11 Jan 2025 at 20:42, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > > > > > Free memory allocated for 'order' (array of bootmeths) on error paths in > > > > > bootmeth_setup_iter_order() function. > > > > > > > > > > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") > > > > > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > > --- > > > > > boot/bootmeth-uclass.c | 16 ++++++++++++---- > > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > How about adding a test to check there is no leak? You can see > > > > test/lib/abuf.c for similar tests. > > > > > > That might be a good idea, and I already have the test code handy. But > > > frankly I wasn't able to successfully run the current bootstd test in > > > sandbox, it fails before it can get to memleak check I added. Firstly, > > > when I do: > > > > > > $ make sandbox_defconfig > > > $ make -j32 > > > $ ./u-boot -T -c "ut bootstd" > > > > > > it says: > > > > > > MMC: Can't map file 'mmc1.img': Invalid argument > > > > > > and I can see only empty (zero size) mmc1.img in my U-Boot source code > > > root dir. Then I was able to trigger its creation (I guess) by running > > > Python suite somehow, so it generated 20 MiB mmc1.img. But even with > > > that added, I hit this error: > > > > > > test/boot/bootdev.c:218, bootdev_test_order(): > > > 0 == bootflow_scan_first(((void *)0), ((void *)0), &iter, 0, &bflow): > > > Expected 0x0 (0), got 0xffffffed (-19) > > > > > > I've read corresponding doc [1] ("Tests" section), but still don't > > > understand how can I successfully run the bootstd test locally. If you > > > can help me out, I can send the memleak test you mentioned in v2. > > > > You can run the bootstd tests with something like: > > > > test/py/test.py -B sandbox --build-dir /tmp/b/sandbox -k bootstd > > > > although it sounds like you have already figured that out. After you > > do it once (to create the images) you can run the C tests directly as > > you did above, but yes, sadly, some of the tests are affected by > > others (with bootstd). You could take a look if you like. > > > > Is it possible to just apply this patch without a test? It's clearly > fixing a memleak. Not that I'm lazy or anything, I came up with a test > like below, but I don't want to send something that is quite hard to > test. If you think this test is really essential, I can send it once > bootstd tests are fixed and can be run in one command without any > extra non-documented work. > > 8<---------------------------------------------------------------------->8 > diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c > index 9af947868707..f4746ee4fb61 100644 > --- a/test/boot/bootdev.c > +++ b/test/boot/bootdev.c > @@ -194,6 +194,7 @@ static int bootdev_test_order(struct unit_test_state *uts) > { > struct bootflow_iter iter; > struct bootflow bflow; > + ulong start; > > test_set_skip_delays(true); > > @@ -201,6 +202,8 @@ static int bootdev_test_order(struct unit_test_state *uts) > bootstd_reset_usb(); > ut_assertok(run_command("usb start", 0)); > > + start = ut_check_free(); > + > /* > * First try the order set by the bootdev-order property > * Like all sandbox unit tests this relies on the devicetree setting up > @@ -277,6 +280,9 @@ static int bootdev_test_order(struct unit_test_state *uts) > iter.dev_used[3]->name); > bootflow_iter_uninit(&iter); > > + /* Check for memory leaks */ > + ut_assertok(ut_check_delta(start)); > + > return 0; > } > BOOTSTD_TEST(bootdev_test_order, UTF_DM | UTF_SCAN_FDT); > 8<---------------------------------------------------------------------->8 Yes, we can do this in a follow-up I think. I'll take this series to -next soon.
On Thu, Jun 19, 2025 at 5:35 PM Tom Rini <trini@konsulko.com> wrote: [snip] > > > > Is it possible to just apply this patch without a test? It's clearly > > fixing a memleak. Not that I'm lazy or anything, I came up with a test > > like below, but I don't want to send something that is quite hard to > > test. If you think this test is really essential, I can send it once > > bootstd tests are fixed and can be run in one command without any > > extra non-documented work. > > [snip] > > Yes, we can do this in a follow-up I think. I'll take this series to > -next soon. > Thanks Tom! It's not very recent though, should I rebase it and re-submit? > -- > Tom
On Thu, Jun 19, 2025 at 09:41:07PM -0500, Sam Protsenko wrote: > On Thu, Jun 19, 2025 at 5:35 PM Tom Rini <trini@konsulko.com> wrote: > > [snip] > > > > > > > Is it possible to just apply this patch without a test? It's clearly > > > fixing a memleak. Not that I'm lazy or anything, I came up with a test > > > like below, but I don't want to send something that is quite hard to > > > test. If you think this test is really essential, I can send it once > > > bootstd tests are fixed and can be run in one command without any > > > extra non-documented work. > > > > > [snip] > > > > > Yes, we can do this in a follow-up I think. I'll take this series to > > -next soon. > > > > Thanks Tom! It's not very recent though, should I rebase it and re-submit? It still applies fine, thanks.
diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c index 5b5fea39b3b3..ff36da78d5a1 100644 --- a/boot/bootmeth-uclass.c +++ b/boot/bootmeth-uclass.c @@ -133,8 +133,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) * We don't support skipping global bootmeths. Instead, the user * should omit them from the ordering */ - if (!include_global) - return log_msg_ret("glob", -EPERM); + if (!include_global) { + ret = log_msg_ret("glob", -EPERM); + goto err_order; + } memcpy(order, std->bootmeth_order, count * sizeof(struct bootmeth *)); @@ -188,8 +190,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) } count = upto; } - if (!count) - return log_msg_ret("count2", -ENOENT); + if (!count) { + ret = log_msg_ret("count2", -ENOENT); + goto err_order; + } if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global && iter->first_glob_method != -1 && iter->first_glob_method != count) { @@ -200,6 +204,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) iter->num_methods = count; return 0; + +err_order: + free(order); + return ret; } int bootmeth_set_order(const char *order_str)
Free memory allocated for 'order' (array of bootmeths) on error paths in bootmeth_setup_iter_order() function. Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately") Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth") Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- boot/bootmeth-uclass.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)