| Message ID | 1680248937-16617-1-git-send-email-quic_srichara@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [V2] net: qrtr: Do not do DEL_SERVER broadcast after DEL_CLIENT | expand |
On Fri, Mar 31, 2023 at 01:18:57PM +0530, Sricharan Ramabadhran wrote: > On the remote side, when QRTR socket is removed, af_qrtr will call > qrtr_port_remove() which broadcasts the DEL_CLIENT packet to all neighbours > including local NS. NS upon receiving the DEL_CLIENT packet, will remove > the lookups associated with the node:port and broadcasts the DEL_SERVER > packet. > > But on the host side, due to the arrival of the DEL_CLIENT packet, the NS > would've already deleted the server belonging to that port. So when the > remote's NS again broadcasts the DEL_SERVER for that port, it throws below > error message on the host: > > "failed while handling packet from 2:-2" > > So fix this error by not broadcasting the DEL_SERVER packet when the > DEL_CLIENT packet gets processed." > > Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace") > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Ram Kumar Dharuman <quic_ramd@quicinc.com> > --- > [v2] Fixed comments from Manivannan and Jakub Kicinski > Note: Functionally tested on 5.4 and compile tested on 6.3 TOT > > net/qrtr/ns.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c > index 722936f..0f25a38 100644 > --- a/net/qrtr/ns.c > +++ b/net/qrtr/ns.c > @@ -274,7 +274,7 @@ static struct qrtr_server *server_add(unsigned int service, > return NULL; > } > > -static int server_del(struct qrtr_node *node, unsigned int port) > +static int server_del(struct qrtr_node *node, unsigned int port, bool bcast) > { > struct qrtr_lookup *lookup; > struct qrtr_server *srv; > @@ -287,7 +287,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) > radix_tree_delete(&node->servers, port); > > /* Broadcast the removal of local servers */ > - if (srv->node == qrtr_ns.local_node) > + if (srv->node == qrtr_ns.local_node && bcast) > service_announce_del(&qrtr_ns.bcast_sq, srv); > > /* Announce the service's disappearance to observers */ > @@ -373,7 +373,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) > } > slot = radix_tree_iter_resume(slot, &iter); > rcu_read_unlock(); > - server_del(node, srv->port); > + server_del(node, srv->port, true); > rcu_read_lock(); > } > rcu_read_unlock(); > @@ -459,10 +459,13 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, > kfree(lookup); > } > > - /* Remove the server belonging to this port */ > + /* Remove the server belonging to this port but don't broadcast This is still not as per the multi line comment style perferred in kernel. Please read: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting - Mani > + * DEL_SERVER. Neighbours would've already removed the server belonging > + * to this port due to the DEL_CLIENT broadcast from qrtr_port_remove(). > + */ > node = node_get(node_id); > if (node) > - server_del(node, port); > + server_del(node, port, false); > > /* Advertise the removal of this client to all local servers */ > local_node = node_get(qrtr_ns.local_node); > @@ -567,7 +570,7 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from, > if (!node) > return -ENOENT; > > - return server_del(node, port); > + return server_del(node, port, true); > } > > static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, > -- > 2.7.4 >
<..> >> >> -static int server_del(struct qrtr_node *node, unsigned int port) >> +static int server_del(struct qrtr_node *node, unsigned int port, bool bcast) >> { >> struct qrtr_lookup *lookup; >> struct qrtr_server *srv; >> @@ -287,7 +287,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) >> radix_tree_delete(&node->servers, port); >> >> /* Broadcast the removal of local servers */ >> - if (srv->node == qrtr_ns.local_node) >> + if (srv->node == qrtr_ns.local_node && bcast) >> service_announce_del(&qrtr_ns.bcast_sq, srv); >> >> /* Announce the service's disappearance to observers */ >> @@ -373,7 +373,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) >> } >> slot = radix_tree_iter_resume(slot, &iter); >> rcu_read_unlock(); >> - server_del(node, srv->port); >> + server_del(node, srv->port, true); >> rcu_read_lock(); >> } >> rcu_read_unlock(); >> @@ -459,10 +459,13 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, >> kfree(lookup); >> } >> >> - /* Remove the server belonging to this port */ >> + /* Remove the server belonging to this port but don't broadcast > > This is still not as per the multi line comment style perferred in kernel. > Please read: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting > Ho, i had it like first style and checkpatch cribbed. Then changed it as per the second style for net/ format. You mean we should stick to 1 st style ? Regards, Sricharan
On Fri, Mar 31, 2023 at 02:02:04PM +0530, Sricharan Ramabadhran wrote: > <..> > > > > -static int server_del(struct qrtr_node *node, unsigned int port) > > > +static int server_del(struct qrtr_node *node, unsigned int port, bool bcast) > > > { > > > struct qrtr_lookup *lookup; > > > struct qrtr_server *srv; > > > @@ -287,7 +287,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) > > > radix_tree_delete(&node->servers, port); > > > /* Broadcast the removal of local servers */ > > > - if (srv->node == qrtr_ns.local_node) > > > + if (srv->node == qrtr_ns.local_node && bcast) > > > service_announce_del(&qrtr_ns.bcast_sq, srv); > > > /* Announce the service's disappearance to observers */ > > > @@ -373,7 +373,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) > > > } > > > slot = radix_tree_iter_resume(slot, &iter); > > > rcu_read_unlock(); > > > - server_del(node, srv->port); > > > + server_del(node, srv->port, true); > > > rcu_read_lock(); > > > } > > > rcu_read_unlock(); > > > @@ -459,10 +459,13 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, > > > kfree(lookup); > > > } > > > - /* Remove the server belonging to this port */ > > > + /* Remove the server belonging to this port but don't broadcast > > > > This is still not as per the multi line comment style perferred in kernel. > > Please read: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting > > > > Ho, i had it like first style and checkpatch cribbed. Then changed it > as per the second style for net/ format. You mean we should stick to > 1 st style ? > Oops, sorry I forgot the fact that the networking code uses a different style. Ignore my above comment. - Mani > Regards, > Sricharan
On Fri, Mar 31, 2023 at 01:18:57PM +0530, Sricharan Ramabadhran wrote: > On the remote side, when QRTR socket is removed, af_qrtr will call > qrtr_port_remove() which broadcasts the DEL_CLIENT packet to all neighbours > including local NS. NS upon receiving the DEL_CLIENT packet, will remove > the lookups associated with the node:port and broadcasts the DEL_SERVER > packet. > > But on the host side, due to the arrival of the DEL_CLIENT packet, the NS > would've already deleted the server belonging to that port. So when the > remote's NS again broadcasts the DEL_SERVER for that port, it throws below > error message on the host: > > "failed while handling packet from 2:-2" > > So fix this error by not broadcasting the DEL_SERVER packet when the > DEL_CLIENT packet gets processed." > > Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace") > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Ram Kumar Dharuman <quic_ramd@quicinc.com> Sender's Signed-off-by should come last. With that fixed, Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> - Mani > --- > [v2] Fixed comments from Manivannan and Jakub Kicinski > Note: Functionally tested on 5.4 and compile tested on 6.3 TOT > > net/qrtr/ns.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c > index 722936f..0f25a38 100644 > --- a/net/qrtr/ns.c > +++ b/net/qrtr/ns.c > @@ -274,7 +274,7 @@ static struct qrtr_server *server_add(unsigned int service, > return NULL; > } > > -static int server_del(struct qrtr_node *node, unsigned int port) > +static int server_del(struct qrtr_node *node, unsigned int port, bool bcast) > { > struct qrtr_lookup *lookup; > struct qrtr_server *srv; > @@ -287,7 +287,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) > radix_tree_delete(&node->servers, port); > > /* Broadcast the removal of local servers */ > - if (srv->node == qrtr_ns.local_node) > + if (srv->node == qrtr_ns.local_node && bcast) > service_announce_del(&qrtr_ns.bcast_sq, srv); > > /* Announce the service's disappearance to observers */ > @@ -373,7 +373,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) > } > slot = radix_tree_iter_resume(slot, &iter); > rcu_read_unlock(); > - server_del(node, srv->port); > + server_del(node, srv->port, true); > rcu_read_lock(); > } > rcu_read_unlock(); > @@ -459,10 +459,13 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, > kfree(lookup); > } > > - /* Remove the server belonging to this port */ > + /* Remove the server belonging to this port but don't broadcast > + * DEL_SERVER. Neighbours would've already removed the server belonging > + * to this port due to the DEL_CLIENT broadcast from qrtr_port_remove(). > + */ > node = node_get(node_id); > if (node) > - server_del(node, port); > + server_del(node, port, false); > > /* Advertise the removal of this client to all local servers */ > local_node = node_get(qrtr_ns.local_node); > @@ -567,7 +570,7 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from, > if (!node) > return -ENOENT; > > - return server_del(node, port); > + return server_del(node, port, true); > } > > static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, > -- > 2.7.4 >
On Fri, Mar 31, 2023 at 02:02:04PM +0530, Sricharan Ramabadhran wrote: > <..> > > > > -static int server_del(struct qrtr_node *node, unsigned int port) > > > +static int server_del(struct qrtr_node *node, unsigned int port, bool bcast) > > > { > > > struct qrtr_lookup *lookup; > > > struct qrtr_server *srv; > > > @@ -287,7 +287,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) > > > radix_tree_delete(&node->servers, port); > > > /* Broadcast the removal of local servers */ > > > - if (srv->node == qrtr_ns.local_node) > > > + if (srv->node == qrtr_ns.local_node && bcast) > > > service_announce_del(&qrtr_ns.bcast_sq, srv); > > > /* Announce the service's disappearance to observers */ > > > @@ -373,7 +373,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) > > > } > > > slot = radix_tree_iter_resume(slot, &iter); > > > rcu_read_unlock(); > > > - server_del(node, srv->port); > > > + server_del(node, srv->port, true); > > > rcu_read_lock(); > > > } > > > rcu_read_unlock(); > > > @@ -459,10 +459,13 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, > > > kfree(lookup); > > > } > > > - /* Remove the server belonging to this port */ > > > + /* Remove the server belonging to this port but don't broadcast > > > > This is still not as per the multi line comment style perferred in kernel. > > Please read: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting > > > > Ho, i had it like first style and checkpatch cribbed. Then changed it > as per the second style for net/ format. You mean we should stick to > 1 st style ? I think that what you have matches the preferred style for net/ code, and thus is correct for this patch. https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#multi-line-comments
diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index 722936f..0f25a38 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -274,7 +274,7 @@ static struct qrtr_server *server_add(unsigned int service, return NULL; } -static int server_del(struct qrtr_node *node, unsigned int port) +static int server_del(struct qrtr_node *node, unsigned int port, bool bcast) { struct qrtr_lookup *lookup; struct qrtr_server *srv; @@ -287,7 +287,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) radix_tree_delete(&node->servers, port); /* Broadcast the removal of local servers */ - if (srv->node == qrtr_ns.local_node) + if (srv->node == qrtr_ns.local_node && bcast) service_announce_del(&qrtr_ns.bcast_sq, srv); /* Announce the service's disappearance to observers */ @@ -373,7 +373,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) } slot = radix_tree_iter_resume(slot, &iter); rcu_read_unlock(); - server_del(node, srv->port); + server_del(node, srv->port, true); rcu_read_lock(); } rcu_read_unlock(); @@ -459,10 +459,13 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, kfree(lookup); } - /* Remove the server belonging to this port */ + /* Remove the server belonging to this port but don't broadcast + * DEL_SERVER. Neighbours would've already removed the server belonging + * to this port due to the DEL_CLIENT broadcast from qrtr_port_remove(). + */ node = node_get(node_id); if (node) - server_del(node, port); + server_del(node, port, false); /* Advertise the removal of this client to all local servers */ local_node = node_get(qrtr_ns.local_node); @@ -567,7 +570,7 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from, if (!node) return -ENOENT; - return server_del(node, port); + return server_del(node, port, true); } static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from,
On the remote side, when QRTR socket is removed, af_qrtr will call qrtr_port_remove() which broadcasts the DEL_CLIENT packet to all neighbours including local NS. NS upon receiving the DEL_CLIENT packet, will remove the lookups associated with the node:port and broadcasts the DEL_SERVER packet. But on the host side, due to the arrival of the DEL_CLIENT packet, the NS would've already deleted the server belonging to that port. So when the remote's NS again broadcasts the DEL_SERVER for that port, it throws below error message on the host: "failed while handling packet from 2:-2" So fix this error by not broadcasting the DEL_SERVER packet when the DEL_CLIENT packet gets processed." Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace") Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> Signed-off-by: Ram Kumar Dharuman <quic_ramd@quicinc.com> --- [v2] Fixed comments from Manivannan and Jakub Kicinski Note: Functionally tested on 5.4 and compile tested on 6.3 TOT net/qrtr/ns.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)