On 14 Oct 2025, at 14:09, Ilya Maximets wrote:
> 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.
Yes, you are right!
>>>> 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
