linux-kernel - 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]

Message-ID: <349a7b1c-915f-4f58-260f-900aa7e3db65@quicinc.com>
Date:   Wed, 20 Sep 2023 17:26:25 -0700
From:   Chris Lew <quic_clew@...cinc.com>
To:     Sricharan Ramabadhran <quic_srichara@...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/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.

>    */
>   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.

> +		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?

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?

Thanks,
Chris