| Message ID | 20200331224348.12539-1-subashab@codeaurora.org |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: qualcomm: rmnet: Allow configuration updates to existing devices | expand |
On 3/31/20 5:43 PM, Subash Abhinov Kasiviswanathan wrote: > This allows the changelink operation to succeed if the mux_id was > specified as an argument. Note that the mux_id must match the > existing mux_id of the rmnet device or should be an unused mux_id. > > Fixes: 1dc49e9d164c ("net: rmnet: do not allow to change mux id if mux id is duplicated") > Reported-by: Alex Elder <elder@linaro.org> > Signed-off-by: Sean Tranchetti <stranche@codeaurora.org> > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> This was a regression in 5.6, and got back-ported to 5.5.11 and possibly further back. Please be sure the fix gets applied to stable branches if appropriate. If you happen to post a second version of this I have a suggestion, below. But the patch looks OK to me as-is. Thanks. Tested-by: Alex Elder <elder@linaro.org> > --- > .../ethernet/qualcomm/rmnet/rmnet_config.c | 21 ++++++++++++------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c > index fbf4cbcf1a65..06332984399d 100644 > --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c > +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c > @@ -294,19 +294,24 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[], > > if (data[IFLA_RMNET_MUX_ID]) { > mux_id = nla_get_u16(data[IFLA_RMNET_MUX_ID]); > - if (rmnet_get_endpoint(port, mux_id)) { > - NL_SET_ERR_MSG_MOD(extack, "MUX ID already exists"); > - return -EINVAL; > - } My suggestion is this: Since the endpoint pointer isn't used outside the "if (mux_id != priv->mux_id)" block, you could do the lookup inside that block. > ep = rmnet_get_endpoint(port, priv->mux_id); > if (!ep) > return -ENODEV; > > - hlist_del_init_rcu(&ep->hlnode); > - hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]); > + if (mux_id != priv->mux_id) { > + if (rmnet_get_endpoint(port, mux_id)) { > + NL_SET_ERR_MSG_MOD(extack, > + "MUX ID already exists"); > + return -EINVAL; > + } > > - ep->mux_id = mux_id; > - priv->mux_id = mux_id; > + hlist_del_init_rcu(&ep->hlnode); > + hlist_add_head_rcu(&ep->hlnode, > + &port->muxed_ep[mux_id]); > + > + ep->mux_id = mux_id; > + priv->mux_id = mux_id; > + } > } > > if (data[IFLA_RMNET_FLAGS]) { >
On 2020-03-31 18:06, Alex Elder wrote: > On 3/31/20 5:43 PM, Subash Abhinov Kasiviswanathan wrote: >> This allows the changelink operation to succeed if the mux_id was >> specified as an argument. Note that the mux_id must match the >> existing mux_id of the rmnet device or should be an unused mux_id. >> >> Fixes: 1dc49e9d164c ("net: rmnet: do not allow to change mux id if mux >> id is duplicated") >> Reported-by: Alex Elder <elder@linaro.org> >> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org> >> Signed-off-by: Subash Abhinov Kasiviswanathan >> <subashab@codeaurora.org> > > This was a regression in 5.6, and got back-ported to 5.5.11 and > possibly further back. Please be sure the fix gets applied to > stable branches if appropriate. > > If you happen to post a second version of this I have a suggestion, > below. But the patch looks OK to me as-is. > > Thanks. > > Tested-by: Alex Elder <elder@linaro.org> > >> --- >> .../ethernet/qualcomm/rmnet/rmnet_config.c | 21 >> ++++++++++++------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c >> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c >> index fbf4cbcf1a65..06332984399d 100644 >> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c >> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c >> @@ -294,19 +294,24 @@ static int rmnet_changelink(struct net_device >> *dev, struct nlattr *tb[], >> >> if (data[IFLA_RMNET_MUX_ID]) { >> mux_id = nla_get_u16(data[IFLA_RMNET_MUX_ID]); >> - if (rmnet_get_endpoint(port, mux_id)) { >> - NL_SET_ERR_MSG_MOD(extack, "MUX ID already exists"); >> - return -EINVAL; >> - } > > My suggestion is this: Since the endpoint pointer isn't used > outside the "if (mux_id != priv->mux_id)" block, you could > do the lookup inside that block. I've sent a v2 now based on your comment.
From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> Date: Tue, 31 Mar 2020 16:43:48 -0600 > This allows the changelink operation to succeed if the mux_id was > specified as an argument. Note that the mux_id must match the > existing mux_id of the rmnet device or should be an unused mux_id. > > Fixes: 1dc49e9d164c ("net: rmnet: do not allow to change mux id if mux id is duplicated") > Reported-by: Alex Elder <elder@linaro.org> > Signed-off-by: Sean Tranchetti <stranche@codeaurora.org> > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> Applied, thanks.
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c index fbf4cbcf1a65..06332984399d 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c @@ -294,19 +294,24 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[], if (data[IFLA_RMNET_MUX_ID]) { mux_id = nla_get_u16(data[IFLA_RMNET_MUX_ID]); - if (rmnet_get_endpoint(port, mux_id)) { - NL_SET_ERR_MSG_MOD(extack, "MUX ID already exists"); - return -EINVAL; - } ep = rmnet_get_endpoint(port, priv->mux_id); if (!ep) return -ENODEV; - hlist_del_init_rcu(&ep->hlnode); - hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]); + if (mux_id != priv->mux_id) { + if (rmnet_get_endpoint(port, mux_id)) { + NL_SET_ERR_MSG_MOD(extack, + "MUX ID already exists"); + return -EINVAL; + } - ep->mux_id = mux_id; - priv->mux_id = mux_id; + hlist_del_init_rcu(&ep->hlnode); + hlist_add_head_rcu(&ep->hlnode, + &port->muxed_ep[mux_id]); + + ep->mux_id = mux_id; + priv->mux_id = mux_id; + } } if (data[IFLA_RMNET_FLAGS]) {
This allows the changelink operation to succeed if the mux_id was specified as an argument. Note that the mux_id must match the existing mux_id of the rmnet device or should be an unused mux_id. Fixes: 1dc49e9d164c ("net: rmnet: do not allow to change mux id if mux id is duplicated") Reported-by: Alex Elder <elder@linaro.org> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> --- .../ethernet/qualcomm/rmnet/rmnet_config.c | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-)