MCUboot firmware update issue

[MariuszSkamra]

Zephyr HEAD: 0b00493
MCUboot HEAD: b97ec0fee8752f3fa376107dd053a7c02384bb8c
MCUmgr-cli HEAD: 073be80c4446a4c9c5102abdb8fadf1ae4a6a627

I've noticed regression in firmware update via mcumgr. After uploading the image one cannot list the available images.
Here are the steps to reproduce:

1. In the MCUboot sources, build boot application

• $ cd <MCUBOOT>/boot/zephyr && mkdir build && cd build
• $ cmake -DBOARD=nrf52840_pca10056 -GNinja ..
• $ ninja
• $ ninja flash
  step_1.log

2. Build smp_svr application form Zephyr sources

• $ cd <ZEPHYR>/samples/subsys/mgmt/mcumgr/smp_svr && mkdir build && cd build
• $ cmake -DBOARD=nrf52840_pca10056 -GNinja ..
• $ ninja
  step_2.log

3. Sign and flash the smp_svr application

• $ cd <MCUBOOT>
• $ ./scripts/imgtool.py sign -k root-rsa-2048.pem --align 8 -H 0x200 --version 1.0 -S 0x69000 ~/projects/zephyr/samples/subsys/mgmt/mcumgr/smp_svr/build/zephyr/zephyr.bin zephyr_signed.bin, where -S 0x69000 is slot size according to boards/arm/nrf52840_pca10056.dts
• $ arm-none-eabi-objcopy --change-address 0xc000 -I binary -O ihex zephyr_signed.bin zephyr_signed.hex, where 0xc000 is the address of slot0 according to boards/arm/nrf52840_pca10056.dts
• $ nrfjprog --program zephyr_signed.hex -f nrf52
• $ nrfjprog -f nrf52 -r
  step_3.log

4. Verify if the device is up and running smp_svr application (via mcumgr-cli)

• $ ./mcumgr --conntype ble --connstring peer_name=Zephyr image list
  step_4.log
  step_4.btmon.log

Up and running, connection tested with nRFconnect on iPhone as well.

5. Change the FW version

• $ cd <MCUBOOT>
• $ ./scripts/imgtool.py sign -k root-rsa-2048.pem --align 8 -H 0x200 --version 2.0 -S 0x69000 ~/projects/zephyr/samples/subsys/mgmt/mcumgr/smp_svr/build/zephyr/zephyr.bin zephyr_signed.bin

6. Upload new FW

• $ ./mcumgr --conntype ble --connstring peer_name=Zephyr image upload ~/projects/mcuboot/zephyr_signed.bin
  step_6.log
  step_6.btmon.log

7. Verify if the device is up and running smp_svr application (via mcumgr-cli)

• $ ./mcumgr --conntype ble --connstring peer_name=Zephyr image list
  step_7.log
  step_7.btmon.log

Got 'Error: NMP timeout' instead.

To get this things working
8. Revert below changes
9823852
cd1111e
9. Repeat steps 1-7
step_9.log
step_9.btmon.log

Images:
slot=0
version: 1.0.0
bootable: true
flags: active confirmed
hash: 52d625f7efe6028c8bc95d1b6ae3723248e81e4a17fb46a357e8aaf2cec787e1
slot=1
version: 2.0.0
bootable: true
flags:
hash: a20ec1215482e11c609f0a0fe4505f3979066ae0aa0d950fc871c0e19d688ef7
Split status: N/A (0)

Image is available in slot 1 as expected.

[nvlsianpu]

So was introduced by #8217.

[nvlsianpu]

I checked that after the image was upload, on incoming image list request bt_conn_unref was called once more than bt_conn_ref.

[jhedberg]

I think the problem is the assymmetry with how buffers are freed. I assumed that the code in smp_bt.c will always take care of freeing buffers that it allocates (smp_bt_buf_free which then calls mcumgr_buf_free). However, it seems that incoming buffers, allocated by smp_bt_chr_write() do not get freed by smp_bt.c, rather by ext/lib/mgmt/mcumgr/smp/src/smp.c when smp_process_request_packet() calls mgmt_streamer_free_buf(). Any freeing that happens outside of smp_bt.c needs to be made aware of the user data that's stored in the buffer, so that it can be properly freed. Is it even necessary to allocate user data and the bt_conn for these buffers? The design and semantics of this code is something very foreign to me, so someone else needs to weigh in.

[carlescufi]

@MariuszSkamra I don't have a setup right now and @nvlsianpu is unfortunately on holiday this week and next. Is there a way you can experiment with the code yourself to try and see what would need to be done to fix this?

[MariuszSkamra]

@carlescufi sure

[jhedberg]

Any update on this? It seems no-one minds this being broken? If it doesn't eventually get fixed my original commit will need to be reverted.

[carlescufi]

@jhedberg I was hoping @MariuszSkamra would investigate the root cause, since @nvlsianpu is currently on holiday.

[MariuszSkamra]

@carlescufi @jhedberg I was looking at this, but I have no solution yet. The problem is that there is no callback to free the user_data (which holds reference to bt_conn in this case). I don't have a patch for this yet, but I would like to fix this by myself.

[jhedberg]

@MariuszSkamra have you verified that the reference is actually needed for these buffers? It might be, but I failed to find the place where it's needed after the point where the buffer leaves smb_bt_c.