| Message ID | 1693563621-1920-2-git-send-email-quic_srichara@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net: qrtr: Few qrtr fixes | expand |
On Fri, Sep 01, 2023 at 03:50:20PM +0530, Sricharan Ramabadhran wrote: > From: Vignesh Viswanathan <quic_viswanat@quicinc.com> > > 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@quicinc.com> > Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> 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 >
Hi Bjorn, On 9/1/2023 7:41 PM, Bjorn Andersson wrote: > On Fri, Sep 01, 2023 at 03:50:20PM +0530, Sricharan Ramabadhran wrote: >> From: Vignesh Viswanathan <quic_viswanat@quicinc.com> >> >> 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". > yes, its name-space server. Will put it explicitly. > 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? > In this case we are talking about a client connecting/sending to QRTR socket and the 'NS' doing a qrtr_bind during its init. There is possibility that a client tries to send to the 'NS' before processing the ENETRESET. In the case of a NEW_SERVER control message will reach the 'NS' and be forwarded to the firmware. The client will then process the ENETRESET closing and re-opening the socket which triggers a DEL_SERVER and then a second NEW_SERVER. This scenario will give an unnecessary disconnect to the clients on the firmware who were able to initialize on the first NEW_SERVER. Also about the patch #2, i guess QRTR_BYE/DEL_PROC should also be broadcasted, right now we are only listening on DEL_PROC sent by legacy kernels like SDX modems. Without that modem SSR feature is broken on IPQ + SDX targets. >> 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@quicinc.com> >> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> > > 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. > ok sure, will fix. > > Let's add Chris to the recipients list as well. > ok. >> --- >> 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? > yup, will be removed in V2. Regards, Sricharan
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; + 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;