On 2013/03/18 09:38, Giovanni Bechis wrote:
> In pfe_filter.c there is a call to DIOCXCOMMIT ioctl to commit pf rules
> generated by relayd but in pfe_filter.c:551 there is only a warning about the
> transaction failing.
> If transaction_commit fails with EBUSY relayd thinks that the pf rules has
> been committed but they are not;
> at this moment relayd is desyncronized.
> With this patch we try to recommit pf rules another time if DIOCXCOMMIT fails
> with EBUSY, otherwise we fatalx.
> I think that it is better to exit the process instead of having a relayd
> process desynchronized.
> Opininions ? Better ideas on how to workaround this problem ?
> Cheers
> Giovanni
> Index: pfe_filter.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/pfe_filter.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 pfe_filter.c
> --- pfe_filter.c 19 Oct 2012 16:49:50 -0000 1.52
> +++ pfe_filter.c 18 Mar 2013 08:06:42 -0000
> @@ -354,9 +354,22 @@ transaction_init(struct relayd *env, con
> int
> transaction_commit(struct relayd *env)
> {
> +int timer = 0;
> +
> if (ioctl(env->sc_pf->dev, DIOCXCOMMIT,
> - &env->sc_pf->pft) == -1)
> - return (-1);
> + &env->sc_pf->pft) == -1) {
> + /*
> + * if DIOCXCOMMIT fails with EBUSY retry after some milliseconds
> + */
> + if(errno == EBUSY) {
> + timer = arc4random_uniform(10000);
> + usleep(timer);
> + if (ioctl(env->sc_pf->dev, DIOCXCOMMIT,
> + &env->sc_pf->pft) == -1)
> + return (-1);
> + }
> + return (0);
> + }
>
> return (0);
> }
> @@ -509,7 +522,7 @@ sync_ruleset(struct relayd *env, struct
> log_debug("%s: rule added to anchor \"%s\"", __func__, anchor);
> }
> if (transaction_commit(env) == -1)
> - log_warn("%s: add rules transaction failed", __func__);
> + fatalx("add rules transaction failed");
> return;
>
> toolong:
Retrying EBUSY definitely makes sense to me.
Wonder if it's worth having a fixed minimum (i.e. "timer = 500 +
arc4random_uniform(10000)" or something)?
I'm undecided about s/log_warn/fatalx/, in some ways it's better
if it keeps operating, in other ways it's better to fail...
Big problem with the diff is that it changes handling for the other
ways in which DIOCXCOMMIT can fail (ENODEV, EFAULT, EINVAL). They are
no longer treated by relayd as a failure.
Also indentation nit in the continuation line of the new "if (ioctl...)".
