On Tue, Feb 1, 2022, at 15:00, Ilya Maximets wrote:
> On 2/1/22 14:38, Gaëtan Rivet wrote:
>> On Tue, Feb 1, 2022, at 12:48, Ilya Maximets wrote:
>>> On 2/1/22 12:38, Gaëtan Rivet wrote:
>>>> On Mon, Jan 31, 2022, at 20:33, Ilya Maximets wrote:
>>>>> On 1/31/22 11:42, Gaëtan Rivet wrote:
>>>>>> On Thu, Jan 27, 2022, at 11:38, Ilya Maximets wrote:
>>>>>>> On 1/27/22 07:42, Sriharsha Basavapatna via dev wrote:
>>>>>>>> In dp_netdev_pmd_remove_flow() we schedule the deletion of an
>>>>>>>> offloaded flow, if a mark has been assigned to the flow. But if
>>>>>>>> this occurs in the window in which the offload thread completes
>>>>>>>> offloading the flow and assigns a mark to the flow, then we miss
>>>>>>>> deleting the flow. This problem has been observed while adding
>>>>>>>> and deleting flows in a loop. To fix this, always enqueue flow
>>>>>>>> deletion regardless of the flow->mark being set.
>>>>>>>>
>>>>>>>> Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
>>>>>>>> Signed-off-by: Sriharsha Basavapatna
>>>>>>>> <[email protected]>
>>>>>>>> ---
>>>>>>>> lib/dpif-netdev.c | 4 +---
>>>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>>>>>>> index e28e0b554..22c5f19a8 100644
>>>>>>>> --- a/lib/dpif-netdev.c
>>>>>>>> +++ b/lib/dpif-netdev.c
>>>>>>>> @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct
>>>>>>>> dp_netdev_pmd_thread *pmd,
>>>>>>>> dp_netdev_simple_match_remove(pmd, flow);
>>>>>>>> cmap_remove(&pmd->flow_table, node,
>>>>>>>> dp_netdev_flow_hash(&flow->ufid));
>>>>>>>> ccmap_dec(&pmd->n_flows, odp_to_u32(in_port));
>>>>>>>> - if (flow->mark != INVALID_FLOW_MARK) {
>>>>>>>> - queue_netdev_flow_del(pmd, flow);
>>>>>>>> - }
>>>>>>>> + queue_netdev_flow_del(pmd, flow);
>>>>>>>> flow->dead = true;
>>>>>>>>
>>>>>>>> dp_netdev_flow_unref(flow);
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for the patch, but it looks like that it's not that simple...
>>>>>>> A lot of tests are crashing in GHA and ASAN reports use-after-free
>>>>>>> on flow disassociation if the dpif already destroyed:
>>>>>>> https://github.com/ovsrobot/ovs/actions/runs/1754866321
>>>>>>>
>>>>>>> I think that the problem is that offload thread holds the ref count
>>>>>>> for PMD thread, but PMD thread structure doesn't hold the ref count
>>>>>>> for the dp, because it doesn't expect that dp_netdev_pmd structure
>>>>>>> will be used while PMD thread is destroyed. I guess, we should fix
>>>>>>> that. OTOH, I'm not sure if offload thread actually needs a reference
>>>>>>> to dp_netdev_pmd structure. If I didn't miss something, it only
>>>>>>> uses pmd pointer to access pmd->dp. So, maybe, dp_offload_thread_item
>>>>>>> should hold and ref the dp pointer instead?
>>>>>>>
>>>>>>> What do you think? Gaetan?
>>>>>>>
>>>>>>
>>>>>> Hi Ilya,
>>>>>>
>>>>>> I've been looking into this issue, you are right that the offload
>>>>>> threads don't
>>>>>> actually need the pmd pointer, only the dp one.
>>>>>>
>>>>>> The problem I see with a proper reference taking on dp to protect
>>>>>> against this
>>>>>> UAF, is that it is prohibitively slow to manage those dp references, and
>>>>>> it relies on
>>>>>> a global lock.
>>>>>>
>>>>>> I have tried to find an alternative (the flush op could be tagged to
>>>>>> mark that
>>>>>> it was issued during a dp_netdev_free), but I always end-up with a
>>>>>> race-condition,
>>>>>> even if the window is reduced.
>>>>> Hmm. Is it with flow API enabled or disabled?
>>>>> With flow api enabled we should have a flush operation that should
>>>>> in theory wait for all the previous offloading operations to be
>>>>> completed. If it's disabled though, flow deletion will initialize
>>>>> the offload thread and enqueue the operation while flush will not
>>>>> actually be requested.
>>>>
>>>> Yes, on your second point, with the API disabled the deletion path relied
>>>> on
>>>> the test flow->mark != INVALID to know whether a flow was offload or not,
>>>> and did not
>>>> care then to check the API status.
>>>> The API state should now be checked as well for the deletion path.
>>>>
>>>>>
>>>>> I missed before that we have flush on that path. Maybe it's enough
>>>>> to just add a !netdev_is_flow_api_enabled() check to the deletion path?
>>>>> Or am I missing something else?
>>>>>
>>>>
>>>> After the flush, there will be remaining ops in the queue that would
>>>> trigger the UAF.
>>>
>>> What are these ops?
>>>
>>
>> Any add/mod/del can be found there.
>>
>> Flushes are scheduled by the main thread in response to user commands.
>> While it executes, PMDs et revalidators are running, potentially enqueuing
>> offload ops. The only restriction on those ops is that it's impossible to
>> have multiple
>> flush in the queue: only one can exist at a time.
>>
>
> I think, revalidators should already be stopped at the point of datapath
> deletion. Don't they?
>
> For the PMDs, I see that they are still running and that sounds like a
> bug, because we're still offloading things after the flush. So, some
> of the flows are still in hardware after the port removal / some userspace
> structures still exist for flows of the deleted datapath.
>
> Can we do something like this (just an untested concept):
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index e28e0b554..4b51ac652 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2313,13 +2313,14 @@ static void
> do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
> OVS_REQ_WRLOCK(dp->port_rwlock)
> {
> - dp_netdev_offload_flush(dp, port);
> - netdev_uninit_flow_api(port->netdev);
> hmap_remove(&dp->ports, &port->node);
> seq_change(dp->port_seq);
>
> reconfigure_datapath(dp);
>
> + dp_netdev_offload_flush(dp, port);
> + netdev_uninit_flow_api(port->netdev);
> +
> port_destroy(port);
> }
>
> ---
> ?
>
> In other words, we should flush flows after removing the port from
> a PMD thread. This way, after removing the last port of a datapath,
> there should be no more offloading operations enqueued after the
> last flush. Of course, assuming that revalidators are stopped at
> this point, which is what I hope for.
> Does that make sense?
>
It makes sense, and it's much better than what I was proposing.
I think it should work.
The only remaining edge-case is with the deletion of a single port.
In that case revalidators will still be running, so after a flush some
offloads might still exist in the queue.
The offload thread will fail to get a netdev ref and will exit early,
so it's fine, but it's something to keep in mind.
I can propose the patches if everyone agrees with this fix.
--
Gaetan Rivet
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
