Skip to content

Bluetooth: controller: Fix Tx Buffer Overflow #31813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 2, 2021
Merged

Bluetooth: controller: Fix Tx Buffer Overflow #31813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c6adb6
Bluetooth: controller: Use DMB instead of DSB
cvinayak Feb 1, 2021
ac8b90d
Bluetooth: controller: Fix Tx Buffer Overflow
cvinayak Feb 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/cpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,25 @@ static inline void cpu_sleep(void)
#endif
}

static inline void cpu_dsb(void)
static inline void cpu_dmb(void)
Copy link
Member

@carlescufi carlescufi Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK for now, but I feel this belongs somewhere in arch/ ?

Copy link
Contributor Author

@cvinayak cvinayak Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will use it, when an interface is available in ARCH, but dont want to go into introducing one now which is going to to be ARM unique.

{
#if defined(CONFIG_CPU_CORTEX_M0) || defined(CONFIG_ARCH_POSIX)
/* No need of data synchronization barrier */
#elif defined(CONFIG_CPU_CORTEX_M4) || defined(CONFIG_CPU_CORTEX_M33)
__DSB();
#if defined(CONFIG_CPU_CORTEX_M)
/* NOTE: Refer to ARM Cortex-M Programming Guide to Memory Barrier
* Instructions, Section 4.1 Normal access in memories
*
* Implementation: In the Cortex-M processors data transfers are
* carried out in the programmed order.
*
* Hence, there is no need to use a memory barrier instruction between
* each access. Only a compiler memory clobber is sufficient.
*/
__asm__ volatile ("" : : : "memory");
#elif defined(CONFIG_ARCH_POSIX)
/* FIXME: Add necessary host machine required Data Memory Barrier
* instruction alongwith the below defined compiler memory
* clobber.
*/
__asm__ volatile ("" : : : "memory");
Copy link
Member

@ioannisg ioannisg Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be applied in the case above, as well?

Copy link
Contributor Author

@cvinayak cvinayak Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves the issue, but following the ARM recommendation, will keep the DMB instruction.

Copy link
Member

@ioannisg ioannisg Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reason not to do this is, if you ever want this code to switch to using arch_ APIs; eventually we're going to get an ARM or even a cross-ARCH API to insert compiler barriers and avoid instruction re-ordering. Then if you already do what's needed here, migrating to generic API calls will be easier and backwards-compatible.

Copy link
Contributor Author

@cvinayak cvinayak Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel code uses DSB/ISB instructions for both M0 and M4 architectures, maybe i understand the control path incorrectly, but they are there definitely in the cpu_idle.S, swap_helper.S to name a few.

When there is a cross-ARCH API available, the change can be done to reuse.

Copy link
Member

@ioannisg ioannisg Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, DSB, ISBs are used, for reasons explained inline (not related to compiler barrier placements)

#else
#error "Unsupported CPU."
#endif
Expand Down
2 changes: 1 addition & 1 deletion subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ struct pdu_adv *lll_adv_pdu_alloc(struct lll_adv_pdu *pdu, uint8_t *idx)
uint8_t first_latest;

pdu->last = first;
cpu_dsb();
cpu_dmb();
first_latest = pdu->first;
if (first_latest != first) {
last++;
Expand Down
9 changes: 5 additions & 4 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,18 @@
#include <stddef.h>

#include <toolchain.h>
#include <soc.h>

#include <sys/util.h>

#include "hal/cpu.h"
#include "hal/ccm.h"
#include "hal/radio.h"

#include "util/mem.h"
#include "util/memq.h"
#include "util/mfifo.h"

#include "hal/ccm.h"
#include "hal/radio.h"

#include "pdu.h"

#include "lll.h"
Expand All @@ -32,7 +34,6 @@
#define BT_DBG_ENABLED IS_ENABLED(CONFIG_BT_DEBUG_HCI_DRIVER)
#define LOG_MODULE_NAME bt_ctlr_lll_conn
#include "common/log.h"
#include <soc.h>
#include "hal/debug.h"

static int init_reset(void);
Expand Down
4 changes: 2 additions & 2 deletions subsys/bluetooth/controller/ll_sw/openisa/lll/lll_adv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,11 @@ struct pdu_adv *lll_adv_pdu_alloc(struct lll_adv_pdu *pdu, uint8_t *idx)
pdu->last = first;
/* FIXME: Ensure that data is synchronized so that an ISR
* vectored, after pdu->last has been updated, does
* access the latest value. __DSB() is used in ARM
* access the latest value. __DMB() is used in ARM
* Cortex M4 architectures. Use appropriate
* instructions on other platforms.
*
* cpu_dsb();
* cpu_dmb();
*/
first_latest = pdu->first;
if (first_latest != first) {
Expand Down
3 changes: 2 additions & 1 deletion subsys/bluetooth/controller/ll_sw/ull_conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@

#include <stddef.h>
#include <zephyr.h>
#include <soc.h>
#include <device.h>
#include <bluetooth/bluetooth.h>
#include <sys/byteorder.h>

#include "hal/cpu.h"
#include "hal/ecb.h"
#include "hal/ccm.h"
#include "hal/ticker.h"
Expand Down Expand Up @@ -44,7 +46,6 @@
#define BT_DBG_ENABLED IS_ENABLED(CONFIG_BT_DEBUG_HCI_DRIVER)
#define LOG_MODULE_NAME bt_ctlr_ull_conn
#include "common/log.h"
#include <soc.h>
#include "hal/debug.h"

#if defined(CONFIG_BT_CTLR_USER_EXT)
Expand Down
8 changes: 6 additions & 2 deletions subsys/bluetooth/controller/util/memq.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@
* where A[b] means the A'th link-element, whose mem pointer is b.
*/

#include <zephyr/types.h>
#include <stddef.h>

#include <soc.h>

#include "hal/cpu.h"

#include "memq.h"

/**
Expand Down Expand Up @@ -97,7 +100,8 @@ memq_link_t *memq_enqueue(memq_link_t *link, void *mem, memq_link_t **tail)
/* Update the tail-pointer to point to the new tail element.
* The new tail-element is not expected to point to anything sensible
*/
*tail = link;
cpu_dmb(); /* Ensure data accesses are synchronized */
*tail = link; /* Commit: enqueue of memq node */

return link;
}
Expand Down
2 changes: 2 additions & 0 deletions subsys/bluetooth/controller/util/mfifo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ static inline void mfifo_by_idx_enqueue(uint8_t *fifo, uint8_t size, uint8_t idx
void **p = (void **)(fifo + (*last) * size); /* buffer preceding idx */
*p = mem; /* store the payload which for API 2 is only a void-ptr */

cpu_dmb(); /* Ensure data accesses are synchronized */
*last = idx; /* Commit: Update write index */
}

Expand Down Expand Up @@ -189,6 +190,7 @@ static inline uint8_t mfifo_enqueue_get(uint8_t *fifo, uint8_t size, uint8_t cou
*/
static inline void mfifo_enqueue(uint8_t idx, uint8_t *last)
{
cpu_dmb(); /* Ensure data accesses are synchronized */
*last = idx; /* Commit: Update write index */
}

Expand Down