netdev - Re: [PATCH V2 net-next 2/2] net: qrtr: Add support for processing DEL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 24 Sep 2023 07:19:18 +0530
From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
To: Chris Lew <quic_clew@...cinc.com>, <mani@...nel.org>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
        <quic_viswanat@...cinc.com>, <horms@...nel.org>
Subject: Re: [PATCH V2 net-next 2/2] net: qrtr: Add support for processing
 DEL_PROC type control message



On 9/21/2023 5:56 AM, Chris Lew wrote:
> 
> On 9/19/2023 10:33 PM, Sricharan Ramabadhran wrote:
> 
>> @@ -122,6 +123,9 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
>>    * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
>>    * @rx_queue: receive queue
>>    * @item: list item for broadcast list
>> + * @kworker: worker thread for recv work
>> + * @task: task to run the worker thread
>> + * @read_data: scheduled work for recv work
> 
> I think I made these descriptions a bit ambiguous with "recv work". 
> Since we are only parsing DEL_PROC messages at the moment, the 
> descriptions should be more accurate on what they are for.

  ok, btw, then would keep your authorship in first place.
  In our downstream, there were multiple changes around here and
  could not get a clear author here. I fixed this while testing
  with Modem SSR recently for our tree, that said, will fix it
  next version.

> 
>>    */
>>   struct qrtr_node {
>>       struct mutex ep_lock;
>> @@ -134,6 +138,9 @@ struct qrtr_node {
>>       struct sk_buff_head rx_queue;
>>       struct list_head item;
>> +    struct kthread_worker kworker;
>> +    struct task_struct *task;
>> +    struct kthread_work read_data;
> 
> I think our own kthread here might have been overkill. I forget why we 
> needed it instead of using a workqueue.

   I added a workqueue here because endpoint post is getting called from
   atomic contexts and below DEL_PROC handling acquires qrtr_tx_lock.

> 
>> +        if (cb->type == QRTR_TYPE_DEL_PROC) {
>> +            /* Free tx flow counters */
>> +            mutex_lock(&node->qrtr_tx_lock);
>> +            radix_tree_for_each_slot(slot, &node->qrtr_tx_flow, 
>> &iter, 0) {
>> +                flow = rcu_dereference_raw(*slot);
>> +                wake_up_interruptible_all(&flow->resume_tx);
>> +            }
>> +            mutex_unlock(&node->qrtr_tx_lock);
>> +
> 
> I don't see any other places where we use rcu_dereference_raw for the 
> flow. Does this need to be updated for the rest of the places we get the 
> flow?
> 
     Yes, without the rcu_dereference_raw there is a SPARSE warning.
     For some reason, did not see the same in other places where flow is
     de-referenced. That said, yeah, will pull this common code and
     create a new helper.

> The same loop is done in qrtr_endpoint_unregister() so maybe we should 
> look into adding a helper for this logic?
> 
>> +            /* Translate DEL_PROC to BYE for local enqueue */
>> +            cb->type = QRTR_TYPE_BYE;
>> +            pkt = (struct qrtr_ctrl_pkt *)skb->data;
>> +            memset(pkt, 0, sizeof(*pkt));
>> +            pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
>> +
> 
> Are we relying on the remote to program the destination of this packet 
> to be the control port?
> 
     Yeah, targets like SDX modems, have a qrtr_fwd_del_proc in the
     endpoint_unregister path.

Regards,
  Sricharan