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?
>> 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
