| Message ID | 20191208142914.19557-1-ophirmu@mellanox.com |
|---|---|
| State | Accepted |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v1] dpif-netdev: Retrieve dpif_class from struct dp_netdev | expand |
On 08.12.2019 15:29, Ophir Munk wrote: > In case a pmd pointer (struct dp_netdev_pmd_thread *) needs to retrieve > the dpif_class it points at - it can access it as: pmd->dp->class. A > second option is to access it as: pmd->dp->dpif->dpif_class. The first > option is safe since there is one dp netdev with a constant pointer to > the dpif class. The second option is not safe since the pointer > pmd->dp->dpif may be changed under the hood, for example, in case there > is a call to dpif_open(). One such scenario is when a netdev bridge is > running while dumping flows statistics with dpctl in parallel: > ovs-appctl dpctl/dump-flows. This commit makes usage of the first > safe option instead of the second option. > > Fixes: 30115809da2e ("dpif-netdev: Use netdev-offload API for port lookup while offloading") > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> Thanks Ophir! I reproduced the issue locally and this patch will fix current case. I'll re-check it and apply to master. However, the root cause is that dpif_netdev_open() updates the 'dpif' pointer on each call that ends up in using of a freed 'dpif'. I'm working on getting rid of this pointer from the dp_netdev strucutre, so we'll not have same issue again. Also, 'prerequisites' patch-set still uses this pointer, so I'll need to update it. Best regards, Ilya Maximets.
On 08.12.2019 15:42, Ilya Maximets wrote: > On 08.12.2019 15:29, Ophir Munk wrote: >> In case a pmd pointer (struct dp_netdev_pmd_thread *) needs to retrieve >> the dpif_class it points at - it can access it as: pmd->dp->class. A >> second option is to access it as: pmd->dp->dpif->dpif_class. The first >> option is safe since there is one dp netdev with a constant pointer to >> the dpif class. The second option is not safe since the pointer >> pmd->dp->dpif may be changed under the hood, for example, in case there >> is a call to dpif_open(). One such scenario is when a netdev bridge is >> running while dumping flows statistics with dpctl in parallel: >> ovs-appctl dpctl/dump-flows. This commit makes usage of the first >> safe option instead of the second option. >> >> Fixes: 30115809da2e ("dpif-netdev: Use netdev-offload API for port lookup while offloading") >> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > > > Thanks Ophir! > > I reproduced the issue locally and this patch will fix current case. > I'll re-check it and apply to master. Applied.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1e54936..7ebf4ef 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -2269,7 +2269,7 @@ mark_to_flow_disassociate(struct dp_netdev_pmd_thread *pmd, struct netdev *port; odp_port_t in_port = flow->flow.in_port.odp_port; - port = netdev_ports_get(in_port, pmd->dp->dpif->dpif_class); + port = netdev_ports_get(in_port, pmd->dp->class); if (port) { ret = netdev_flow_del(port, &flow->mega_ufid, NULL); netdev_close(port); @@ -2410,7 +2410,7 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item *offload) } info.flow_mark = mark; - port = netdev_ports_get(in_port, pmd->dp->dpif->dpif_class); + port = netdev_ports_get(in_port, pmd->dp->class); if (!port || netdev_vport_is_vport_class(port->netdev_class)) { netdev_close(port); goto err_free;
In case a pmd pointer (struct dp_netdev_pmd_thread *) needs to retrieve the dpif_class it points at - it can access it as: pmd->dp->class. A second option is to access it as: pmd->dp->dpif->dpif_class. The first option is safe since there is one dp netdev with a constant pointer to the dpif class. The second option is not safe since the pointer pmd->dp->dpif may be changed under the hood, for example, in case there is a call to dpif_open(). One such scenario is when a netdev bridge is running while dumping flows statistics with dpctl in parallel: ovs-appctl dpctl/dump-flows. This commit makes usage of the first safe option instead of the second option. Fixes: 30115809da2e ("dpif-netdev: Use netdev-offload API for port lookup while offloading") Signed-off-by: Ophir Munk <ophirmu@mellanox.com> --- lib/dpif-netdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)