| Message ID | 20201211092738.3358-1-simon.horman@netronome.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] nfp: do not send control messages during cleanup | expand |
On Fri, 11 Dec 2020 10:27:38 +0100 Simon Horman wrote: > On cleanup the txbufs are freed before app cleanup. But app clean-up may > result in control messages due to use of common control paths. There is no > need to clean-up the NIC in such cases so simply discard requests. Without > such a check a NULL pointer dereference occurs. > > Fixes: a1db217861f3 ("net: flow_offload: fix flow_indr_dev_unregister path") > Cc: wenxu <wenxu@ucloud.cn> > Signed-off-by: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Louis Peens <louis.peens@netronome.com> Hm. We can apply this as a quick fix, but I'd think that app->stop (IIRC that's the callback) is responsible for making sure that everything gets shut down and no more cmsgs can be generated after ctrl vNIC goes down. Perhaps some code needs to be reshuffled between init/clean and start/stop for flower? WDYT?
On Mon, Dec 14, 2020 at 06:26:50PM -0800, Jakub Kicinski wrote: > On Fri, 11 Dec 2020 10:27:38 +0100 Simon Horman wrote: > > On cleanup the txbufs are freed before app cleanup. But app clean-up may > > result in control messages due to use of common control paths. There is no > > need to clean-up the NIC in such cases so simply discard requests. Without > > such a check a NULL pointer dereference occurs. > > > > Fixes: a1db217861f3 ("net: flow_offload: fix flow_indr_dev_unregister path") > > Cc: wenxu <wenxu@ucloud.cn> > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > Signed-off-by: Louis Peens <louis.peens@netronome.com> > > Hm. We can apply this as a quick fix, but I'd think that app->stop > (IIRC that's the callback) is responsible for making sure that > everything gets shut down and no more cmsgs can be generated after > ctrl vNIC goes down. Perhaps some code needs to be reshuffled between > init/clean and start/stop for flower? WDYT? Thanks Jakub, I was a bit concerned with fragility in the clean-up path, which is why I had opted for this simple solution. However, looking at your suggestion above it seems simple to move the cleanup to app->stop. I'll work on posting a patch to implement your suggestion.
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c index b4acf2f41e84..d86f68aa89bf 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c @@ -2084,6 +2084,15 @@ nfp_ctrl_tx_one(struct nfp_net *nn, struct nfp_net_r_vector *r_vec, dp = &r_vec->nfp_net->dp; tx_ring = r_vec->tx_ring; + if (!tx_ring->txbufs) + /* On cleanup the txbufs are freed before app cleanup. + * But app clean-up may result in control messages due to + * use of common control paths. There is no need to + * clean-up the NIC in such cases so simply discard + * requests. + */ + goto err_free; + if (WARN_ON_ONCE(skb_shinfo(skb)->nr_frags)) { nn_dp_warn(dp, "Driver's CTRL TX does not implement gather\n"); goto err_free;
On cleanup the txbufs are freed before app cleanup. But app clean-up may result in control messages due to use of common control paths. There is no need to clean-up the NIC in such cases so simply discard requests. Without such a check a NULL pointer dereference occurs. Fixes: a1db217861f3 ("net: flow_offload: fix flow_indr_dev_unregister path") Cc: wenxu <wenxu@ucloud.cn> Signed-off-by: Simon Horman <simon.horman@netronome.com> Signed-off-by: Louis Peens <louis.peens@netronome.com> --- drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 9 +++++++++ 1 file changed, 9 insertions(+)