From:   "=?utf-8?Q?Micha=C5=82?= Lowas-Rzechonek" 
        <michal.lowas-rzechonek@silvair.com>
Date:   Wed, 24 Apr 2019 10:59:45 +0200
To:     Brian Gix <brian.gix@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, inga.stotland@intel.com
Subject: Re: [PATCH BlueZ] mesh: Add remote dev key support and cleanup
Message-ID: <20190424085945.zmniib4vpy2uiyql@kynes>
Mail-Followup-To: Brian Gix <brian.gix@intel.com>,
        linux-bluetooth@vger.kernel.org, inga.stotland@intel.com
References: <20190423231624.19302-1-brian.gix@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190423231624.19302-1-brian.gix@intel.com>
User-Agent: NeoMutt/20180716
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

A few comments.

On 04/23, Brian Gix wrote:
> +try_remote:
> +	/* Try remote device key if available */
> +	dev_key = node_get_remote_device_key(node, src);
> +	if (!dev_key)
> +		return -1;
> +
> +	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> +					dst, key_id, seq, iv_idx, out, dev_key))
> +		return APP_IDX_DEV_RMT;
> +
What's the purpose of APP_IDX_DEV_RMT?

I'm not a fan of magic values (and I hope APP_IDX_DEV goes away afterw
we manage to implement proposed Config/Provisioning API), but at least
APP_IDX_DEV is used to indicate 'privileged' loopback messages.

Adding IDX_DEV_RMT doesn't seem necessary.

> diff --git a/mesh/node.c b/mesh/node.c
> index 157991dad..fe0488108 100644
> --- a/mesh/node.c
> +++ b/mesh/node.c
> @@ -54,6 +54,8 @@
>  #define DEFAULT_CRPL 10
>  #define DEFAULT_SEQUENCE_NUMBER 0
>  
> +#define REMOTE_CACHE_LEN 5
By the way: why the limits here are set so low (and there doesn't seem
to be a way to change them...)? I don't think we should put artificial
constraints on network size, so both CRPL and remote cache could safely
be set to cover full unicast space (32768). In any practical
application, the device running Linux + bluetooth will have more than
enough RAM for this.

Getting rid of these constraints would make the implementation
substiantially simpler (no MRU, for example).

FYI, we're running networks with couples of hundreds devices, and plan
to expand them even further.

> +/* This is a thread-safe always malloced version of dirname which will work
> + * regardless of which underlying dirname implimentation is used
> + */
> +static char *alloc_dirname(const char *path)
> +{
I was under impression that mesh daemon operates inside a main loop, so
it doesn't need to worry about thread safety?

> +bool storage_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,
> +							uint8_t dev_key[16])
> +{
> (...)
> +	dir_name = alloc_dirname(cfgname);
I don't see why the remote key storage should be kept separately (and in
binary files?) from the main node json. It makes the implementation much
more complicated, as you need to worry about directories etc.

I believie that keeping remote keys in the same place, as a JSON object,
would be much simpler.

regards
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND