Re: [PATCH 2/2] netfilter: conntrack: optional reliable conntrack event delivery

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> I'll do in a couple of minutes: I have another issue that you're going
> to notice in the new patchset, I put it here before posting them if you
> have time to look at it. I'd like to know if you're OK with this.
> 
> events = xchg(&e->cache, 0);
> [...]
> -               notify->fcn(events, &item);
> +               ret = notify->fcn(events, &item);
> +               if (likely(ret == 0))
> +                       delivered = 1;
> +       }
> +       if (unlikely(!delivered)) {
> +               unsigned int old = 0;
> +retry:
> +               /* restore undelivered events to the cache */
> +               ret = cmpxchg(&e->cache, old, events);
> +               /* ... but we've got new events during delivery */
> +               if (unlikely(ret != old)) {
> +                       old = ret;
> +                       events |= ret;
> +                       goto retry;
> +               }
>         }
> 
>  out_unlock:
> 
> To avoid races between the cache clearing and event delivery:
> 
> 1) I retrieve the event cache and clear it with xchg.
> 2) Then, if I fail to deliver the event, I restore the cache. However,
> if the event cache is not zero anymore, then some new events have been
> cached during the delivery, I use cmpxchg to conditionally restore the
> events without losing the new events.
> 
> Can you see any problem with this optimistic approach? I know, it can
> potentially try to restore the cache, but this is very unlikely to happen?

Its probably quite unlikely, so not a big issue. OTOH this is
effectively doing something quite similar to a spinlock. Maybe
its finally time to add per-conntrack locking.

Eric even had a patch for this IIRC :)