On Thu, 28 Sep 2023, Florian Westphal wrote: > Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> wrote: > > On Thu, 28 Sep 2023, Pablo Neira Ayuso wrote: > > > One concern might be deadlock due to reordering, but I don't see how > > > that can happen. > > > > The same problem exists ipset: when a set is listed/saved (dumped), > > concurrent destroy/rename/swap for the same set must be excluded. As > > neither spinlock nor mutex helps, a reference counter is used: the start > > of the dump increases it and by checking it all concurrent events can > > safely be rejected by returning EBUSY. > > Thanks for sharing! > > I assume that means that a dumper that starts a dump, and then > goes to sleep before closing the socket/finishing the dump can > block further ipset updates, is that correct? Concurrent updates are supported, both from user and kernel space. The operations which would lead to corruption are excluded, like destroying the set being dumped or swapping the dumped set with another one. A relatively new application of the method is when there's a huge add-del operation which must be temporarily halted and the scheduler called (so spinlock/mutex cannot be held): for that time destroy/swap must also be excluded. > (I assume so, I don't see a solution that doesn't trade one problem > for another). Yes, usually that's the case. However, this code segment triggered me: > > > > + mutex_lock(&nft_net->commit_mutex); > > > > + ret = nf_tables_dump_set(skb, cb); > > > > + mutex_unlock(&nft_net->commit_mutex); > > > > + > > > > + if (cb->args[0] > skip) > > > > + audit_log_nft_set_reset(dump_ctx->ctx.table, cb->seq, > > > > + cb->args[0] - skip); > > > > + That can quite nicely be handled by reference counting the object and protecting that way. Best regards, Jozsef - E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxx PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics H-1525 Budapest 114, POB. 49, Hungary
