Re: [PATCH net-next 1/2] net: qrtr: Prevent stale ports from sending

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 03:50:20PM +0530, Sricharan Ramabadhran wrote:
> From: Vignesh Viswanathan <quic_viswanat@xxxxxxxxxxx>
> 
> If qrtr and some other process try to bind to the QMI Control port at

It's unclear to me which "qrtr" is being referred here, could it be
"qrtr-ns", if so could we express that as "the name server".

We only allow one bind on the qrtr control port, so could it be that
"QMI Control port" refer to the control socket in the userspace QC[CS]I
libraries, if so that's just any random socket sending out a control
message.

Can we please rephrase this problem description to make the chain of
events clear?

> the same time, NEW_SERVER might come before ENETRESET is given to the
> socket. This might cause a socket down/up when ENETRESET is received as
> per the protocol and this triggers a DEL_SERVER and a second NEW_SERVER.
> 
> In order to prevent such messages from stale sockets being sent, check
> if ENETRESET has been set on the socket and drop the packet.
> 
> Signed-off-by: Chris Lew <quic_clew@xxxxxxxxxxx>
> Signed-off-by: Vignesh Viswanathan <quic_viswanat@xxxxxxxxxxx>

The first person to certify the patch's origin, must be the author, and
when you pick the patch to send it you need to add your s-o-b.

So please fix the author, and add your s-o-b.


Let's add Chris to the recipients list as well.

> ---
>  net/qrtr/af_qrtr.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 41ece61..26197a0 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -851,6 +851,7 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  {
>  	struct qrtr_sock *ipc;
>  	struct qrtr_cb *cb;
> +	struct sock *sk = skb->sk;
>  
>  	ipc = qrtr_port_lookup(to->sq_port);
>  	if (!ipc || &ipc->sk == skb->sk) { /* do not send to self */
> @@ -860,6 +861,15 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  		return -ENODEV;
>  	}
>  
> +	/* Keep resetting NETRESET until socket is closed */
> +	if (sk && sk->sk_err == ENETRESET) {
> +		sk->sk_err = ENETRESET;

Isn't this line unnecessary?

Regards,
Bjorn

> +		sk_error_report(sk);
> +		qrtr_port_put(ipc);
> +		kfree_skb(skb);
> +		return 0;
> +	}
> +
>  	cb = (struct qrtr_cb *)skb->cb;
>  	cb->src_node = from->sq_node;
>  	cb->src_port = from->sq_port;
> -- 
> 2.7.4
>