The current implementation of QRTR assumes that each entity on the QRTR
IPC bus is uniquely identifiable by its node/port combination, with
node/port combinations being used to route messages between entities.

However, this assumption of uniqueness is problematic in scenarios
where multiple devices with the same node/port combinations are
connected to the system.  A practical example is a typical consumer PC
with multiple PCIe-based devices, such as WiFi cards or 5G modems, where
each device could potentially have the same node identifier set.  In
such cases, the current QRTR protocol implementation does not provide a
mechanism to differentiate between these devices, making it impossible
to support communication with multiple identical devices.

This patch series addresses this limitation by introducing support for
a concept of an 'endpoint.' Multiple devices with conflicting node/port
combinations can be supported by assigning a unique endpoint identifier
to each one.  Such endpoint identifiers can then be used to distinguish
between devices while sending and receiving messages over QRTR sockets.

The patch series maintains backward compatibility with existing clients:
the endpoint concept is added using auxiliary data that can be added to
recvmsg and sendmsg system calls.  The QRTR socket interface is extended
as follows:

- Adds QRTR_ENDPOINT auxiliary data element that reports which endpoint
  generated a particular message.  This auxiliary data is only reported
  if the socket was explicitly opted in using setsockopt, enabling the
  QRTR_REPORT_ENDPOINT socket option.  SOL_QRTR socket level was added
  to facilitate this.  This requires QRTR clients to be updated to use
  recvmsg instead of the more typical recvfrom() or recv() use.

- Similarly, QRTR_ENDPOINT auxiliary data element can be included in
  sendmsg() requests.  This will allow clients to route QRTR messages
  to the desired endpoint, even in cases of node/port conflict between
  multiple endpoints.

- Finally, QRTR_BIND_ENDPOINT socket option is introduced.  This allows
  clients to bind to a particular endpoint (such as a 5G PCIe modem) if
  they're only interested in receiving or sending messages to this
  device.

NOTE: There is 32-bit unsafe use of radix_tree_insert in this patch set.
This follows the existing usage inside net/qrtr/af_qrtr.c in
qrtr_tx_wait(), qrtr_tx_resume() and qrtr_tx_flow_failed().  This was
done deliberately in order to keep the changes as minimal as possible
until it is known whether the approach outlined is generally acceptable.

Denis Kenzior (10):
  net: qrtr: ns: validate msglen before ctrl_pkt use
  net: qrtr: allocate and track endpoint ids
  net: qrtr: support identical node ids
  net: qrtr: Report sender endpoint in aux data
  net: qrtr: Report endpoint for locally generated messages
  net: qrtr: Allow sendmsg to target an endpoint
  net: qrtr: allow socket endpoint binding
  net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages
  net: qrtr: ns: support multiple endpoints
  net: qrtr: mhi: Report endpoint id in sysfs

 include/linux/socket.h    |   1 +
 include/uapi/linux/qrtr.h |   7 +
 net/qrtr/af_qrtr.c        | 297 +++++++++++++++++++++++++++++++------
 net/qrtr/mhi.c            |  14 ++
 net/qrtr/ns.c             | 299 +++++++++++++++++++++++---------------
 net/qrtr/qrtr.h           |   4 +
 6 files changed, 459 insertions(+), 163 deletions(-)

-- 
2.45.2

On 10/18/2024 11:18 AM, Denis Kenzior wrote:
> The current implementation of QRTR assumes that each entity on the QRTR
> IPC bus is uniquely identifiable by its node/port combination, with
> node/port combinations being used to route messages between entities.
> 
> However, this assumption of uniqueness is problematic in scenarios
> where multiple devices with the same node/port combinations are
> connected to the system.  A practical example is a typical consumer PC
> with multiple PCIe-based devices, such as WiFi cards or 5G modems, where
> each device could potentially have the same node identifier set.  In
> such cases, the current QRTR protocol implementation does not provide a
> mechanism to differentiate between these devices, making it impossible
> to support communication with multiple identical devices.
> 
> This patch series addresses this limitation by introducing support for
> a concept of an 'endpoint.' Multiple devices with conflicting node/port
> combinations can be supported by assigning a unique endpoint identifier
> to each one.  Such endpoint identifiers can then be used to distinguish
> between devices while sending and receiving messages over QRTR sockets.
> 
> The patch series maintains backward compatibility with existing clients:
> the endpoint concept is added using auxiliary data that can be added to
> recvmsg and sendmsg system calls.  The QRTR socket interface is extended
> as follows:
> 
> - Adds QRTR_ENDPOINT auxiliary data element that reports which endpoint
>    generated a particular message.  This auxiliary data is only reported
>    if the socket was explicitly opted in using setsockopt, enabling the
>    QRTR_REPORT_ENDPOINT socket option.  SOL_QRTR socket level was added
>    to facilitate this.  This requires QRTR clients to be updated to use
>    recvmsg instead of the more typical recvfrom() or recv() use.
> 
> - Similarly, QRTR_ENDPOINT auxiliary data element can be included in
>    sendmsg() requests.  This will allow clients to route QRTR messages
>    to the desired endpoint, even in cases of node/port conflict between
>    multiple endpoints.
> 
> - Finally, QRTR_BIND_ENDPOINT socket option is introduced.  This allows
>    clients to bind to a particular endpoint (such as a 5G PCIe modem) if
>    they're only interested in receiving or sending messages to this
>    device.
> 
> NOTE: There is 32-bit unsafe use of radix_tree_insert in this patch set.
> This follows the existing usage inside net/qrtr/af_qrtr.c in
> qrtr_tx_wait(), qrtr_tx_resume() and qrtr_tx_flow_failed().  This was
> done deliberately in order to keep the changes as minimal as possible
> until it is known whether the approach outlined is generally acceptable.
> 

Hi Denis,

Thank you for taking a stab at this long standing problem. We've been 
going back and forth on how to solve this but haven't had anyone 
dedicated to working out a solution. From a first pass I think this 
looks very reasonable and I only have a few nitpicks here and there. 
Hopefully Bjorn and Mani will provide more feedback.

Thanks!
Chris


> Denis Kenzior (10):
>    net: qrtr: ns: validate msglen before ctrl_pkt use
>    net: qrtr: allocate and track endpoint ids
>    net: qrtr: support identical node ids
>    net: qrtr: Report sender endpoint in aux data
>    net: qrtr: Report endpoint for locally generated messages
>    net: qrtr: Allow sendmsg to target an endpoint
>    net: qrtr: allow socket endpoint binding
>    net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages
>    net: qrtr: ns: support multiple endpoints
>    net: qrtr: mhi: Report endpoint id in sysfs
> 
>   include/linux/socket.h    |   1 +
>   include/uapi/linux/qrtr.h |   7 +
>   net/qrtr/af_qrtr.c        | 297 +++++++++++++++++++++++++++++++------
>   net/qrtr/mhi.c            |  14 ++
>   net/qrtr/ns.c             | 299 +++++++++++++++++++++++---------------
>   net/qrtr/qrtr.h           |   4 +
>   6 files changed, 459 insertions(+), 163 deletions(-)
>

On Fri, Oct 18, 2024 at 01:18:18PM -0500, Denis Kenzior wrote:
> The current implementation of QRTR assumes that each entity on the QRTR
> IPC bus is uniquely identifiable by its node/port combination, with
> node/port combinations being used to route messages between entities.
> 
> However, this assumption of uniqueness is problematic in scenarios
> where multiple devices with the same node/port combinations are
> connected to the system.  A practical example is a typical consumer PC
> with multiple PCIe-based devices, such as WiFi cards or 5G modems, where
> each device could potentially have the same node identifier set.  In
> such cases, the current QRTR protocol implementation does not provide a
> mechanism to differentiate between these devices, making it impossible
> to support communication with multiple identical devices.
> 
> This patch series addresses this limitation by introducing support for
> a concept of an 'endpoint.' Multiple devices with conflicting node/port
> combinations can be supported by assigning a unique endpoint identifier
> to each one.  Such endpoint identifiers can then be used to distinguish
> between devices while sending and receiving messages over QRTR sockets.
> 

Thanks for your work on this! I'm yet to look into the details but wondering how
all the patches have Reviewed-by tags provided that this series is 'RFC v1'.
Also, it is quite surprising to see the review tag from Andy Gross who quit Qcom
quite a while ago and I haven't seen him reviewing any Qcom patches for so long.

- Mani

> The patch series maintains backward compatibility with existing clients:
> the endpoint concept is added using auxiliary data that can be added to
> recvmsg and sendmsg system calls.  The QRTR socket interface is extended
> as follows:
> 
> - Adds QRTR_ENDPOINT auxiliary data element that reports which endpoint
>   generated a particular message.  This auxiliary data is only reported
>   if the socket was explicitly opted in using setsockopt, enabling the
>   QRTR_REPORT_ENDPOINT socket option.  SOL_QRTR socket level was added
>   to facilitate this.  This requires QRTR clients to be updated to use
>   recvmsg instead of the more typical recvfrom() or recv() use.
> 
> - Similarly, QRTR_ENDPOINT auxiliary data element can be included in
>   sendmsg() requests.  This will allow clients to route QRTR messages
>   to the desired endpoint, even in cases of node/port conflict between
>   multiple endpoints.
> 
> - Finally, QRTR_BIND_ENDPOINT socket option is introduced.  This allows
>   clients to bind to a particular endpoint (such as a 5G PCIe modem) if
>   they're only interested in receiving or sending messages to this
>   device.
> 
> NOTE: There is 32-bit unsafe use of radix_tree_insert in this patch set.
> This follows the existing usage inside net/qrtr/af_qrtr.c in
> qrtr_tx_wait(), qrtr_tx_resume() and qrtr_tx_flow_failed().  This was
> done deliberately in order to keep the changes as minimal as possible
> until it is known whether the approach outlined is generally acceptable.
> 
> Denis Kenzior (10):
>   net: qrtr: ns: validate msglen before ctrl_pkt use
>   net: qrtr: allocate and track endpoint ids
>   net: qrtr: support identical node ids
>   net: qrtr: Report sender endpoint in aux data
>   net: qrtr: Report endpoint for locally generated messages
>   net: qrtr: Allow sendmsg to target an endpoint
>   net: qrtr: allow socket endpoint binding
>   net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages
>   net: qrtr: ns: support multiple endpoints
>   net: qrtr: mhi: Report endpoint id in sysfs
> 
>  include/linux/socket.h    |   1 +
>  include/uapi/linux/qrtr.h |   7 +
>  net/qrtr/af_qrtr.c        | 297 +++++++++++++++++++++++++++++++------
>  net/qrtr/mhi.c            |  14 ++
>  net/qrtr/ns.c             | 299 +++++++++++++++++++++++---------------
>  net/qrtr/qrtr.h           |   4 +
>  6 files changed, 459 insertions(+), 163 deletions(-)
> 
> -- 
> 2.45.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Hi Mani,

On 10/22/24 10:39 AM, Manivannan Sadhasivam wrote:
> On Fri, Oct 18, 2024 at 01:18:18PM -0500, Denis Kenzior wrote:
>> The current implementation of QRTR assumes that each entity on the QRTR
>> IPC bus is uniquely identifiable by its node/port combination, with
>> node/port combinations being used to route messages between entities.
>>
>> However, this assumption of uniqueness is problematic in scenarios
>> where multiple devices with the same node/port combinations are
>> connected to the system.  A practical example is a typical consumer PC
>> with multiple PCIe-based devices, such as WiFi cards or 5G modems, where
>> each device could potentially have the same node identifier set.  In
>> such cases, the current QRTR protocol implementation does not provide a
>> mechanism to differentiate between these devices, making it impossible
>> to support communication with multiple identical devices.
>>
>> This patch series addresses this limitation by introducing support for
>> a concept of an 'endpoint.' Multiple devices with conflicting node/port
>> combinations can be supported by assigning a unique endpoint identifier
>> to each one.  Such endpoint identifiers can then be used to distinguish
>> between devices while sending and receiving messages over QRTR sockets.
>>
> 
> Thanks for your work on this! I'm yet to look into the details but wondering how
> all the patches have Reviewed-by tags provided that this series is 'RFC v1'.
> Also, it is quite surprising to see the review tag from Andy Gross who quit Qcom
> quite a while ago and I haven't seen him reviewing any Qcom patches for so long.
> 

I have very limited experience in kernel development, so the first few 
iterations were shared privately with a few folks to make sure I wasn't 
completely off base.  Andy was one of them :)

Regards,
-Denis

> - Mani
> 
>> The patch series maintains backward compatibility with existing clients:
>> the endpoint concept is added using auxiliary data that can be added to
>> recvmsg and sendmsg system calls.  The QRTR socket interface is extended
>> as follows:
>>
>> - Adds QRTR_ENDPOINT auxiliary data element that reports which endpoint
>>    generated a particular message.  This auxiliary data is only reported
>>    if the socket was explicitly opted in using setsockopt, enabling the
>>    QRTR_REPORT_ENDPOINT socket option.  SOL_QRTR socket level was added
>>    to facilitate this.  This requires QRTR clients to be updated to use
>>    recvmsg instead of the more typical recvfrom() or recv() use.
>>
>> - Similarly, QRTR_ENDPOINT auxiliary data element can be included in
>>    sendmsg() requests.  This will allow clients to route QRTR messages
>>    to the desired endpoint, even in cases of node/port conflict between
>>    multiple endpoints.
>>
>> - Finally, QRTR_BIND_ENDPOINT socket option is introduced.  This allows
>>    clients to bind to a particular endpoint (such as a 5G PCIe modem) if
>>    they're only interested in receiving or sending messages to this
>>    device.
>>
>> NOTE: There is 32-bit unsafe use of radix_tree_insert in this patch set.
>> This follows the existing usage inside net/qrtr/af_qrtr.c in
>> qrtr_tx_wait(), qrtr_tx_resume() and qrtr_tx_flow_failed().  This was
>> done deliberately in order to keep the changes as minimal as possible
>> until it is known whether the approach outlined is generally acceptable.
>>
>> Denis Kenzior (10):
>>    net: qrtr: ns: validate msglen before ctrl_pkt use
>>    net: qrtr: allocate and track endpoint ids
>>    net: qrtr: support identical node ids
>>    net: qrtr: Report sender endpoint in aux data
>>    net: qrtr: Report endpoint for locally generated messages
>>    net: qrtr: Allow sendmsg to target an endpoint
>>    net: qrtr: allow socket endpoint binding
>>    net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages
>>    net: qrtr: ns: support multiple endpoints
>>    net: qrtr: mhi: Report endpoint id in sysfs
>>
>>   include/linux/socket.h    |   1 +
>>   include/uapi/linux/qrtr.h |   7 +
>>   net/qrtr/af_qrtr.c        | 297 +++++++++++++++++++++++++++++++------
>>   net/qrtr/mhi.c            |  14 ++
>>   net/qrtr/ns.c             | 299 +++++++++++++++++++++++---------------
>>   net/qrtr/qrtr.h           |   4 +
>>   6 files changed, 459 insertions(+), 163 deletions(-)
>>
>> -- 
>> 2.45.2
>>
>