| Message ID | 20200407205611.1002903-1-marcel@holtmann.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | Bluetooth: Update resolving list when updating whitelist | expand |
Hi Marcel, On Tue, Apr 7, 2020 at 1:57 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > When the whitelist is updated, then also update the entries of the > resolving list for devices where IRKs are available. > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > --- > net/bluetooth/hci_request.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index efec2a0bb824..45fbda5323af 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -695,6 +695,21 @@ static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr, > bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr, > cp.bdaddr_type); > hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp); > + > + if (use_ll_privacy(req->hdev)) { > + struct smp_irk *irk; > + > + irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type); > + if (irk) { > + struct hci_cp_le_del_from_resolv_list cp; > + > + cp.bdaddr_type = bdaddr_type; > + bacpy(&cp.bdaddr, bdaddr); > + > + hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST, > + sizeof(cp), &cp); > + } > + } > } > > /* Adds connection to white list if needed. On error, returns -1. */ > @@ -715,7 +730,7 @@ static int add_to_white_list(struct hci_request *req, > return -1; > > /* White list can not be used with RPAs */ > - if (!allow_rpa && > + if (!allow_rpa && !use_ll_privacy(hdev) && > hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) { > return -1; > } > @@ -732,6 +747,24 @@ static int add_to_white_list(struct hci_request *req, > cp.bdaddr_type); > hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp); > > + if (use_ll_privacy(hdev)) { > + struct smp_irk *irk; > + > + irk = hci_find_irk_by_addr(hdev, ¶ms->addr, > + params->addr_type); > + if (irk) { > + struct hci_cp_le_add_to_resolv_list cp; > + > + cp.bdaddr_type = params->addr_type; > + bacpy(&cp.bdaddr, ¶ms->addr); > + memcpy(cp.peer_irk, irk->val, 16); > + memset(cp.local_irk, 0, 16); > + > + hci_req_add(req, HCI_OP_LE_ADD_TO_RESOLV_LIST, > + sizeof(cp), &cp); Shouldn't we be checking if there is any space left in the list before trying to send the command? I wonder what would happen if there is too many IRKs, I guess that means we would still have to resolve them in the host. > + } > + } > + > return 0; > } > > @@ -772,7 +805,7 @@ static u8 update_white_list(struct hci_request *req) > } > > /* White list can not be used with RPAs */ > - if (!allow_rpa && > + if (!allow_rpa && !use_ll_privacy(hdev) && > hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) { > return 0x00; > } > -- > 2.25.2 >
Hi Luiz, >> When the whitelist is updated, then also update the entries of the >> resolving list for devices where IRKs are available. >> >> Signed-off-by: Marcel Holtmann <marcel@holtmann.org> >> --- >> net/bluetooth/hci_request.c | 37 +++++++++++++++++++++++++++++++++++-- >> 1 file changed, 35 insertions(+), 2 deletions(-) >> >> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c >> index efec2a0bb824..45fbda5323af 100644 >> --- a/net/bluetooth/hci_request.c >> +++ b/net/bluetooth/hci_request.c >> @@ -695,6 +695,21 @@ static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr, >> bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr, >> cp.bdaddr_type); >> hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp); >> + >> + if (use_ll_privacy(req->hdev)) { >> + struct smp_irk *irk; >> + >> + irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type); >> + if (irk) { >> + struct hci_cp_le_del_from_resolv_list cp; >> + >> + cp.bdaddr_type = bdaddr_type; >> + bacpy(&cp.bdaddr, bdaddr); >> + >> + hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST, >> + sizeof(cp), &cp); >> + } >> + } >> } >> >> /* Adds connection to white list if needed. On error, returns -1. */ >> @@ -715,7 +730,7 @@ static int add_to_white_list(struct hci_request *req, >> return -1; >> >> /* White list can not be used with RPAs */ >> - if (!allow_rpa && >> + if (!allow_rpa && !use_ll_privacy(hdev) && >> hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) { >> return -1; >> } >> @@ -732,6 +747,24 @@ static int add_to_white_list(struct hci_request *req, >> cp.bdaddr_type); >> hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp); >> >> + if (use_ll_privacy(hdev)) { >> + struct smp_irk *irk; >> + >> + irk = hci_find_irk_by_addr(hdev, ¶ms->addr, >> + params->addr_type); >> + if (irk) { >> + struct hci_cp_le_add_to_resolv_list cp; >> + >> + cp.bdaddr_type = params->addr_type; >> + bacpy(&cp.bdaddr, ¶ms->addr); >> + memcpy(cp.peer_irk, irk->val, 16); >> + memset(cp.local_irk, 0, 16); >> + >> + hci_req_add(req, HCI_OP_LE_ADD_TO_RESOLV_LIST, >> + sizeof(cp), &cp); > > Shouldn't we be checking if there is any space left in the list before > trying to send the command? I wonder what would happen if there is too > many IRKs, I guess that means we would still have to resolve them in > the host. the resolving list size is just a guesstimate and no guarantee that space is available. However my assumption is that if you have x entries for whitespace, then you also get x entries for the resolving list. If we are not putting the device in the whitelist, then it also makes no sense to add it to the resolving list. Regards Marcel
Hi Marcel, Since the resolv list is tied directly to the whitelist, do we still need hdev->le_resolv_list? Maybe we should remove it. Abhishek On Tue, Apr 7, 2020 at 1:56 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > When the whitelist is updated, then also update the entries of the > resolving list for devices where IRKs are available. > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > --- > net/bluetooth/hci_request.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index efec2a0bb824..45fbda5323af 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -695,6 +695,21 @@ static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr, > bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr, > cp.bdaddr_type); > hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp); > + > + if (use_ll_privacy(req->hdev)) { > + struct smp_irk *irk; > + > + irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type); > + if (irk) { > + struct hci_cp_le_del_from_resolv_list cp; > + > + cp.bdaddr_type = bdaddr_type; > + bacpy(&cp.bdaddr, bdaddr); > + > + hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST, > + sizeof(cp), &cp); > + } > + } > } > > /* Adds connection to white list if needed. On error, returns -1. */ > @@ -715,7 +730,7 @@ static int add_to_white_list(struct hci_request *req, > return -1; > > /* White list can not be used with RPAs */ > - if (!allow_rpa && > + if (!allow_rpa && !use_ll_privacy(hdev) && > hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) { > return -1; > } > @@ -732,6 +747,24 @@ static int add_to_white_list(struct hci_request *req, > cp.bdaddr_type); > hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp); > > + if (use_ll_privacy(hdev)) { > + struct smp_irk *irk; > + > + irk = hci_find_irk_by_addr(hdev, ¶ms->addr, > + params->addr_type); > + if (irk) { > + struct hci_cp_le_add_to_resolv_list cp; > + > + cp.bdaddr_type = params->addr_type; > + bacpy(&cp.bdaddr, ¶ms->addr); > + memcpy(cp.peer_irk, irk->val, 16); > + memset(cp.local_irk, 0, 16); > + > + hci_req_add(req, HCI_OP_LE_ADD_TO_RESOLV_LIST, > + sizeof(cp), &cp); > + } > + } > + > return 0; > } > > @@ -772,7 +805,7 @@ static u8 update_white_list(struct hci_request *req) > } > > /* White list can not be used with RPAs */ > - if (!allow_rpa && > + if (!allow_rpa && !use_ll_privacy(hdev) && > hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) { > return 0x00; > } > -- > 2.25.2 >
Hi Marcel,
I love your patch! Yet something to improve:
[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on next-20200407]
[cannot apply to v5.6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Marcel-Holtmann/Bluetooth-Update-resolving-list-when-updating-whitelist/20200408-045729
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
net/bluetooth/hci_request.c: In function 'del_from_white_list':
>> net/bluetooth/hci_request.c:693:6: error: implicit declaration of function 'use_ll_privacy' [-Werror=implicit-function-declaration]
693 | if (use_ll_privacy(req->hdev)) {
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/use_ll_privacy +693 net/bluetooth/hci_request.c
680
681 static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
682 u8 bdaddr_type)
683 {
684 struct hci_cp_le_del_from_white_list cp;
685
686 cp.bdaddr_type = bdaddr_type;
687 bacpy(&cp.bdaddr, bdaddr);
688
689 bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr,
690 cp.bdaddr_type);
691 hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
692
> 693 if (use_ll_privacy(req->hdev)) {
694 struct smp_irk *irk;
695
696 irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type);
697 if (irk) {
698 struct hci_cp_le_del_from_resolv_list cp;
699
700 cp.bdaddr_type = bdaddr_type;
701 bacpy(&cp.bdaddr, bdaddr);
702
703 hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST,
704 sizeof(cp), &cp);
705 }
706 }
707 }
708
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Abhishek, > Since the resolv list is tied directly to the whitelist, do we still > need hdev->le_resolv_list? Maybe we should remove it. it is really useful for debugging since you can look into debugfs to see what is actually programmed into the controller. I also have the feeling that eventually we might need the resolving list when we have privacy enabled. Regards Marcel
Hi Marcel On Wed, Apr 8, 2020 at 2:26 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > When the whitelist is updated, then also update the entries of the > resolving list for devices where IRKs are available. > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > --- > net/bluetooth/hci_request.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index efec2a0bb824..45fbda5323af 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -695,6 +695,21 @@ static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr, > bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr, > cp.bdaddr_type); > hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp); > + > + if (use_ll_privacy(req->hdev)) { > + struct smp_irk *irk; > + > + irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type); > + if (irk) { > + struct hci_cp_le_del_from_resolv_list cp; > + > + cp.bdaddr_type = bdaddr_type; > + bacpy(&cp.bdaddr, bdaddr); > + > + hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST, > + sizeof(cp), &cp); > + } > + } > } > > /* Adds connection to white list if needed. On error, returns -1. */ > @@ -715,7 +730,7 @@ static int add_to_white_list(struct hci_request *req, > return -1; > > /* White list can not be used with RPAs */ > - if (!allow_rpa && > + if (!allow_rpa && !use_ll_privacy(hdev) && > hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) { > return -1; > } > @@ -732,6 +747,24 @@ static int add_to_white_list(struct hci_request *req, > cp.bdaddr_type); > hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp); > > + if (use_ll_privacy(hdev)) { > + struct smp_irk *irk; > + > + irk = hci_find_irk_by_addr(hdev, ¶ms->addr, > + params->addr_type); > + if (irk) { > + struct hci_cp_le_add_to_resolv_list cp; > + > + cp.bdaddr_type = params->addr_type; > + bacpy(&cp.bdaddr, ¶ms->addr); > + memcpy(cp.peer_irk, irk->val, 16); > + memset(cp.local_irk, 0, 16); Shall we memcpy hdev->irk as local_irk here. if privacy (HCI_PRIVACY) is enabled. also if controller supports LL_PRIVACY so that stack can utilize it. else continue with HCI_PRIVACY. Is there any drawback with this approach. please guide > + > + hci_req_add(req, HCI_OP_LE_ADD_TO_RESOLV_LIST, > + sizeof(cp), &cp); > + } > + } > + > return 0; > } > > @@ -772,7 +805,7 @@ static u8 update_white_list(struct hci_request *req) > } > > /* White list can not be used with RPAs */ > - if (!allow_rpa && > + if (!allow_rpa && !use_ll_privacy(hdev) && > hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) { > return 0x00; > } > -- > 2.25.2 > Regards Sathish N
Hi Marcel On Tue, May 5, 2020 at 6:41 PM Sathish Narasimman <nsathish41@gmail.com> wrote: > > Hi Marcel > > On Wed, Apr 8, 2020 at 2:26 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > > > When the whitelist is updated, then also update the entries of the > > resolving list for devices where IRKs are available. > > > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > > --- > > net/bluetooth/hci_request.c | 37 +++++++++++++++++++++++++++++++++++-- > > 1 file changed, 35 insertions(+), 2 deletions(-) > > > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > > index efec2a0bb824..45fbda5323af 100644 > > --- a/net/bluetooth/hci_request.c > > +++ b/net/bluetooth/hci_request.c > > @@ -695,6 +695,21 @@ static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr, > > bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr, > > cp.bdaddr_type); > > hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp); > > + > > + if (use_ll_privacy(req->hdev)) { > > + struct smp_irk *irk; > > + > > + irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type); > > + if (irk) { > > + struct hci_cp_le_del_from_resolv_list cp; > > + > > + cp.bdaddr_type = bdaddr_type; > > + bacpy(&cp.bdaddr, bdaddr); > > + > > + hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST, > > + sizeof(cp), &cp); > > + } > > + } > > } > > > > /* Adds connection to white list if needed. On error, returns -1. */ > > @@ -715,7 +730,7 @@ static int add_to_white_list(struct hci_request *req, > > return -1; > > > > /* White list can not be used with RPAs */ > > - if (!allow_rpa && > > + if (!allow_rpa && !use_ll_privacy(hdev) && > > hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) { > > return -1; > > } > > @@ -732,6 +747,24 @@ static int add_to_white_list(struct hci_request *req, > > cp.bdaddr_type); > > hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp); > > > > + if (use_ll_privacy(hdev)) { > > + struct smp_irk *irk; > > + > > + irk = hci_find_irk_by_addr(hdev, ¶ms->addr, > > + params->addr_type); > > + if (irk) { > > + struct hci_cp_le_add_to_resolv_list cp; > > + > > + cp.bdaddr_type = params->addr_type; > > + bacpy(&cp.bdaddr, ¶ms->addr); > > + memcpy(cp.peer_irk, irk->val, 16); > > + memset(cp.local_irk, 0, 16); > Shall we memcpy hdev->irk as local_irk here. > if privacy (HCI_PRIVACY) is enabled. also if controller supports > LL_PRIVACY so that stack can utilize it. > else continue with HCI_PRIVACY. > Is there any drawback with this approach. please guide > > + > > + hci_req_add(req, HCI_OP_LE_ADD_TO_RESOLV_LIST, > > + sizeof(cp), &cp); > > + } > > + } > > + > > return 0; > > } > > > > @@ -772,7 +805,7 @@ static u8 update_white_list(struct hci_request *req) > > } > > > > /* White list can not be used with RPAs */ > > - if (!allow_rpa && > > + if (!allow_rpa && !use_ll_privacy(hdev) && > > hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) { > > return 0x00; > > } > > -- > > 2.25.2 > > I have uploaded patch v2 with a slight modification over your patch. can you please review > > Regards > Sathish N Regards Sathish N
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index efec2a0bb824..45fbda5323af 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -695,6 +695,21 @@ static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr, bt_dev_dbg(req->hdev, "Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type); hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp); + + if (use_ll_privacy(req->hdev)) { + struct smp_irk *irk; + + irk = hci_find_irk_by_addr(req->hdev, bdaddr, bdaddr_type); + if (irk) { + struct hci_cp_le_del_from_resolv_list cp; + + cp.bdaddr_type = bdaddr_type; + bacpy(&cp.bdaddr, bdaddr); + + hci_req_add(req, HCI_OP_LE_DEL_FROM_RESOLV_LIST, + sizeof(cp), &cp); + } + } } /* Adds connection to white list if needed. On error, returns -1. */ @@ -715,7 +730,7 @@ static int add_to_white_list(struct hci_request *req, return -1; /* White list can not be used with RPAs */ - if (!allow_rpa && + if (!allow_rpa && !use_ll_privacy(hdev) && hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) { return -1; } @@ -732,6 +747,24 @@ static int add_to_white_list(struct hci_request *req, cp.bdaddr_type); hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp); + if (use_ll_privacy(hdev)) { + struct smp_irk *irk; + + irk = hci_find_irk_by_addr(hdev, ¶ms->addr, + params->addr_type); + if (irk) { + struct hci_cp_le_add_to_resolv_list cp; + + cp.bdaddr_type = params->addr_type; + bacpy(&cp.bdaddr, ¶ms->addr); + memcpy(cp.peer_irk, irk->val, 16); + memset(cp.local_irk, 0, 16); + + hci_req_add(req, HCI_OP_LE_ADD_TO_RESOLV_LIST, + sizeof(cp), &cp); + } + } + return 0; } @@ -772,7 +805,7 @@ static u8 update_white_list(struct hci_request *req) } /* White list can not be used with RPAs */ - if (!allow_rpa && + if (!allow_rpa && !use_ll_privacy(hdev) && hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) { return 0x00; }
When the whitelist is updated, then also update the entries of the resolving list for devices where IRKs are available. Signed-off-by: Marcel Holtmann <marcel@holtmann.org> --- net/bluetooth/hci_request.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)