Subject: Re: [PATCH RFC v6 net-next 13/17] p4tc: add table entry create, update, get, delete, flush and dump

From: Vlad Buslov <[email-masked]>
Date: Mon, 9 Oct 2023 17:27:00 +0300

On Mon 09 Oct 2023 at 10:02, Jamal Hadi Salim <[email-masked]> wrote:
> Hi Vlad,
>
> On Sun, Oct 8, 2023 at 12:36 PM Vlad Buslov <[email-masked]> wrote:
>>
>> On Sat 30 Sep 2023 at 10:35, Jamal Hadi Salim <[email-masked]> wrote:
>
> [..trimmed...]
>
>> > +/* Invoked from both control and data path  */
>> > +static int __p4tc_table_entry_update(struct p4tc_pipeline *pipeline,
>> > +                                  struct p4tc_table *table,
>> > +                                  struct p4tc_table_entry *entry,
>> > +                                  struct p4tc_table_entry_mask *mask,
>> > +                                  u16 whodunnit, bool from_control)
>> > +__must_hold(RCU)
>> > +{
>> > +     struct p4tc_table_entry_mask *mask_found = NULL;
>> > +     struct p4tc_table_entry_work *entry_work;
>> > +     struct p4tc_table_entry_value *value_old;
>> > +     struct p4tc_table_entry_value *value;
>> > +     struct p4tc_table_entry *entry_old;
>> > +     struct p4tc_table_entry_tm *tm_old;
>> > +     struct p4tc_table_entry_tm *tm;
>> > +     int ret;
>> > +
>> > +     value = p4tc_table_entry_value(entry);
>> > +     /* We set it to zero on create an update to avoid having entry
>> > +      * deletion in parallel before we report to user space.
>> > +      */
>> > +     refcount_set(&value->entries_ref, 0);
>>
>> TBH I already commented on one of the previous versions of this series
>> that it is very hard to understand and review tons of different atomic
>> reference counters, especially when they are modified with functions
>> like refcount_dec_not_one() or unconditional set like in this place.
>>
>> I chose specifically this function because __must_hold(RCU) makes it
>> look like it can be accessed concurrently from datapath, which is not
>> obvious on multiple previous usages of reference counters in the series.
>
> True, tables can be manipulated from control plane/user space,
> datapath as well as timers (mostly for delete).
> Would using wrappers around these incr/decr help? i mean meaningful
> inlines that will provide clarity as to what an incr/decr is?

Considering existing wrappers I don't believe adding more would help.
Looking at wrappers for entries_ref:

static inline int p4tc_tbl_entry_get(struct p4tc_table_entry_value *value)
{
	return refcount_inc_not_zero(&value->entries_ref);
}

static inline bool p4tc_tbl_entry_put(struct p4tc_table_entry_value *value)
{
	return refcount_dec_if_one(&value->entries_ref);
}

static inline bool p4tc_tbl_entry_put_ref(struct p4tc_table_entry_value *value)
{
	return refcount_dec_not_one(&value->entries_ref);
}

it isn't obvious to me why 'put' is dec_if_one and put_ref is dec_not
one.

>
>> So what happens here if entries_ref was 0 to begin with? Or is possible
>> for this function to be executed concurrently by multiple tasks, in
>> which case all of them set entries_ref to 0, but first one that finishes
>> resets the counter back to 1 at which point I assume it can be deleted
>> in parallel by control path while some concurrent
>> __p4tc_table_entry_update() are still running (at least that is what the
>> comment here indicates)?
>
> It's rtnl-lockless, so you can imagine what would happen there ;->
> Multiple concurent user space, kernel, timers all contending for this.
> Exactly what you said: its zero in this case because some entity could
> delete it in parallel.
> See comment further down which says "In case of parallel update, the
> thread that arrives here first will..."

See below.

> Consider it a poor man's lock. Does that help? Perhaps we could have
> more discussion at the monthly tc meetup..
> We have been testing this code a lot for concurrency and wrote some
> user space tooling to catch such issues.

I'm not arguing that it is somehow broken or not well-tested, just
saying I read this and previous versions several times and still have no
idea what is going on here.

>
>>
>> > +
>> > +     if (table->tbl_type != P4TC_TABLE_TYPE_EXACT) {
>> > +             mask_found = p4tc_table_entry_mask_add(table, entry, mask);
>> > +             if (IS_ERR(mask_found)) {
>> > +                     ret = PTR_ERR(mask_found);
>> > +                     goto out;
>> > +             }
>> > +     }
>> > +

Mark A, see next comment.

>> > +     p4tc_table_entry_build_key(table, &entry->key, mask_found);
>> > +
>> > +     entry_old = p4tc_entry_lookup(table, &entry->key, value->prio);
>> > +     if (!entry_old) {
>> > +             ret = -ENOENT;
>> > +             goto rm_masks_idr;
>> > +     }
>> > +
>> > +     /* In case of parallel update, the thread that arrives here first will
>> > +      * get the right to update.
>> > +      *
>> > +      * In case of a parallel get/update, whoever is second will fail appropriately
>> > +      */
>> > +     value_old = p4tc_table_entry_value(entry_old);
>> > +     if (!p4tc_tbl_entry_put(value_old)) {
>> > +             ret = -EAGAIN;

I get that this prevents you from updating the old entry in parallel,
but I was asking what would happen if concurrent update already finished
by this time, set value->entries_ref to 1 and concurrent delete
deallocated the new entry? Isn't use after free possible when calling
p4tc_table_entry_build_key() and p4tc_entry_lookup() if all those events
happen before mark A? (for example, if task spent a lot of time
allocating new mask in p4tc_table_entry_mask_add())

>> > +             goto rm_masks_idr;
>> > +     }
>> > +
>> > +     if (from_control) {
>> > +             if (!p4tc_ctrl_update_ok(value_old->permissions)) {
>> > +                     ret = -EPERM;
>> > +                     goto set_entries_refcount;
>> > +             }
>> > +     } else {
>> > +             if (!p4tc_data_update_ok(value_old->permissions)) {
>> > +                     ret = -EPERM;
>> > +                     goto set_entries_refcount;
>> > +             }
>> > +     }
>> > +
>> > +     tm = kzalloc(sizeof(*tm), GFP_ATOMIC);
>> > +     if (unlikely(!tm)) {
>> > +             ret = -ENOMEM;
>> > +             goto set_entries_refcount;
>> > +     }
>> > +
>> > +     tm_old = rcu_dereference_protected(value_old->tm, 1);
>> > +     *tm = *tm_old;
>> > +
>> > +     tm->lastused = jiffies;
>> > +     tm->who_updated = whodunnit;
>> > +
>> > +     if (value->permissions == P4TC_PERMISSIONS_UNINIT)
>> > +             value->permissions = value_old->permissions;
>> > +
>> > +     rcu_assign_pointer(value->tm, tm);
>> > +
>> > +     entry_work = kzalloc(sizeof(*(entry_work)), GFP_ATOMIC);
>> > +     if (unlikely(!entry_work)) {
>> > +             ret = -ENOMEM;
>> > +             goto free_tm;
>> > +     }
>> > +
>> > +     entry_work->pipeline = pipeline;
>> > +     entry_work->table = table;
>> > +     entry_work->entry = entry;
>> > +     value->entry_work = entry_work;
>> > +     if (!value->is_dyn)
>> > +             value->is_dyn = value_old->is_dyn;
>> > +
>> > +     if (value->is_dyn) {
>> > +             /* Only use old entry value if user didn't specify new one */
>> > +             value->aging_ms = value->aging_ms ?: value_old->aging_ms;
>> > +
>> > +             hrtimer_init(&value->entry_timer, CLOCK_MONOTONIC,
>> > +                          HRTIMER_MODE_REL);
>> > +             value->entry_timer.function = &entry_timer_handle;
>> > +
>> > +             hrtimer_start(&value->entry_timer, ms_to_ktime(value->aging_ms),
>> > +                           HRTIMER_MODE_REL);
>> > +     }
>> > +
>> > +     INIT_WORK(&entry_work->work, p4tc_table_entry_del_work);
>> > +
>> > +     if (rhltable_insert(&table->tbl_entries, &entry->ht_node,
>> > +                         entry_hlt_params) < 0) {
>> > +             ret = -EEXIST;
>> > +             goto free_entry_work;
>> > +     }
>> > +
>> > +     p4tc_table_entry_destroy_noida(table, entry_old);
>> > +
>> > +     if (!from_control)
>> > +             p4tc_tbl_entry_emit_event(entry_work, RTM_P4TC_UPDATE,
>> > +                                       GFP_ATOMIC);
>> > +
>> > +     return 0;
>> > +
>> > +free_entry_work:
>> > +     kfree(entry_work);
>> > +
>> > +free_tm:
>> > +     kfree(tm);
>> > +
>> > +set_entries_refcount:
>> > +     refcount_set(&value_old->entries_ref, 1);
>> > +
>> > +rm_masks_idr:
>> > +     if (table->tbl_type != P4TC_TABLE_TYPE_EXACT)
>> > +             p4tc_table_entry_mask_del(table, entry);
>> > +
>> > +out:
>> > +     return ret;
>> > +}
>> > +
>
> [..trimmed..]
>
>  cheers,
> jamal