On 10/14/25 1:39 PM, Eelco Chaudron wrote:
>
>
> On 14 Oct 2025, at 13:12, Ilya Maximets wrote:
>
>> On 10/14/25 12:09 PM, Kevin Traynor via dev wrote:
>>> On 14/10/2025 10:15, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 14 Oct 2025, at 11:11, Kevin Traynor wrote:
>>>>
>>>>> On 13/10/2025 13:32, Eelco Chaudron via dev wrote:
>>>>>> This patch fixes a potential null pointer dereference reported
>>>>>> by Coverity if an null actions list is passed to nl_attr_get()
>>>>>> in odp_execute_sample().
>>>>>>
>>>>>> Fixes: 26c6b6cd2b2e ("dpif-netdev: Implement OVS_ACTION_ATTR_SAMPLE
>>>>>> action.")
>>>>>> Signed-off-by: Eelco Chaudron <[email protected]>
>>>>>> ---
>>>>>> v2: Actually check for the actions non-null
>>>>>> ---
>>>>>> lib/odp-execute.c | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>>>>>> index 7f4e337f8..8943db2a5 100644
>>>>>> --- a/lib/odp-execute.c
>>>>>> +++ b/lib/odp-execute.c
>>>>>> @@ -739,6 +739,13 @@ odp_execute_sample(void *dp, struct dp_packet
>>>>>> *packet, bool steal,
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> + if (!subactions || !nl_attr_get_size(subactions)) {
>>>>>> + if (steal) {
>>>>>> + dp_packet_delete(packet);
>>>>>
>>>>> Seems like it needs the coverage counter increased here as well
>>>>>
>>>>> COVERAGE_INC(datapath_drop_sample_error);
>>>>
>>>> Good catch, I can fold that in when applying. I’ll wait a bit longer
>>>> to see if any other comments come in.
>>
>> FWIW, the original datapath_drop_sample_error makes no sense as is not
>> an error to drop a packet based on probability, it's just how the action
>> works. I'd say this counter shouldn't have existed. But it is suitable
>> for this new condition, which is actually an error if there are no
>> actions in the sample. At the same time if the action list exists, but
>> it is empty, I'm not sure that qualifies as error, it can be a normal
>> condition.
>>
>> Best regards, Ilya Maximets.
>
> Ok, I added the !nl_attr_get_size(subactions) as an optimization to avoid
> allocating + freeing the packet without any actions. Maybe we should remove
> it, and only cover the error case?
>
> + if (!subactions) {
> + if (steal) {
> + dp_packet_delete(packet);
> + }
> + COVERAGE_INC(datapath_drop_sample_error);
> + return;
> + }
> +
>
> WDYT?
Looks fine, but we should probably move the counter under the if statement,
as we shouldn't count this as a packet drop if it's not.
>
>>> ok, thanks. With coverage counter,
>>>
>>> Acked-by: Kevin Traynor <[email protected]>
>>>
>>>> Thanks,
>>>>
>>>> Eelco
>>>>
>>>>>> + }
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> if (!steal) {
>>>>>> /* The 'subactions' may modify the packet, but the modification
>>>>>> * should not propagate beyond this sample action. Make a copy
>>>>
>>>
>>> _______________________________________________
>>> dev mailing list
>>> [email protected]
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
