[ovs-dev,2/2] ofproto-dpif-upcall: Fix key attr iteration.

This call is operating on messages generated by the datapath. If a
datapath implementation sends improperly formatted netlink attributes,
then it's possible for a revalidator thread to end up trapped in an
infinite loop iterating across these attributes. Rather than using the
UNSAFE variation of this iterator, use the regular version.

Fixes: 994fcc5a15d3 ("upcall: Check for recirc_id in ukey_create_from_dpif_flow()")
Signed-off-by: Joe Stringer <joe@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 07/31/2017 04:54 PM, Joe Stringer wrote:
> This call is operating on messages generated by the datapath. If a
> datapath implementation sends improperly formatted netlink attributes,
> then it's possible for a revalidator thread to end up trapped in an
> infinite loop iterating across these attributes. Rather than using the
> UNSAFE variation of this iterator, use the regular version.
>
> Fixes: 994fcc5a15d3 ("upcall: Check for recirc_id in ukey_create_from_dpif_flow()")
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
>   ofproto/ofproto-dpif-upcall.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 8d1783accdc8..56773686404b 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1600,7 +1600,7 @@ ukey_create_from_dpif_flow(const struct udpif *udpif,
>        * relies on OVS userspace internal state, we need to delete all old
>        * datapath flows with either a non-zero recirc_id in the key, or any
>        * recirculation actions upon OVS restart. */
> -    NL_ATTR_FOR_EACH_UNSAFE (a, left, flow->key, flow->key_len) {
> +    NL_ATTR_FOR_EACH (a, left, flow->key, flow->key_len) {
>           if (nl_attr_type(a) == OVS_KEY_ATTR_RECIRC_ID
>               && nl_attr_get_u32(a) != 0) {
>               return EINVAL;
>
Reviewed-by: Greg Rose <gvrose8192@gmail.com>

On Mon, Jul 31, 2017 at 04:54:22PM -0700, Joe Stringer wrote:
> This call is operating on messages generated by the datapath. If a
> datapath implementation sends improperly formatted netlink attributes,
> then it's possible for a revalidator thread to end up trapped in an
> infinite loop iterating across these attributes. Rather than using the
> UNSAFE variation of this iterator, use the regular version.
> 
> Fixes: 994fcc5a15d3 ("upcall: Check for recirc_id in ukey_create_from_dpif_flow()")
> Signed-off-by: Joe Stringer <joe@ovn.org>

I wonder whether there's enough benefit in the _UNSAFE variants to keep
them around.

Acked-by: Ben Pfaff <blp@ovn.org>

On 3 August 2017 at 14:35, Ben Pfaff <blp@ovn.org> wrote:
> On Mon, Jul 31, 2017 at 04:54:22PM -0700, Joe Stringer wrote:
>> This call is operating on messages generated by the datapath. If a
>> datapath implementation sends improperly formatted netlink attributes,
>> then it's possible for a revalidator thread to end up trapped in an
>> infinite loop iterating across these attributes. Rather than using the
>> UNSAFE variation of this iterator, use the regular version.
>>
>> Fixes: 994fcc5a15d3 ("upcall: Check for recirc_id in ukey_create_from_dpif_flow()")
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>
> I wonder whether there's enough benefit in the _UNSAFE variants to keep
> them around.

Hmm. It's used in odp-execute to iterate generated actions, that's the
primary place that could have some sensitivity to this as it's used in
the userdp fastpath. If we can't measure an impact there with a decent
average actions list length, then perhaps it could be removed
altogether.

> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks for the reviews, applied to master and branches 2.5-2.8.