Bug 1646666

Created attachment 1502217 [details]
minimal patch

Hi,

I've created the attached minimal patch that seems to solve the problem.

By reverse sorting the merged file before loading, all create statements are sorted before the add statements. This way all sets are already created before they are added by sets of the list:set type.

Regards,
Rik

Thanks for reporting this, Rik.

Hi,

I'm confused. This bug is marked as regression and urgent, but is only targeted for 7.7? Isn't that a contradiction?

This bug can (silently) break existing firewall setups upon upgrade to 7.6. It would be awesome if it could be fixed in 7.6 as well.

Regards,
Rik

Hi Rik,

(In reply to Rik Theys from comment #7)
> I'm confused. This bug is marked as regression and urgent, but is only
> targeted for 7.7? Isn't that a contradiction?
> 
> This bug can (silently) break existing firewall setups upon upgrade to 7.6.
> It would be awesome if it could be fixed in 7.6 as well.

Engineering has internally requested a z-stream fix for 7.6. The target release does not prevent this bug from moving forward.

By the way,

(In reply to Rik Theys from comment #2)
> I've created the attached minimal patch that seems to solve the problem.
> 
> By reverse sorting the merged file before loading, all create statements are
> sorted before the add statements. This way all sets are already created
> before they are added by sets of the list:set type.

Thanks for your patch. While it definitely fixes the issue, it doesn't scale too well for sets containing a huge number of entries, because of the added computational complexity given by sort(1) applied on all the entries.

I benchmarked a few different solutions, and ended up with something similar, that doesn't however require sorting all the entries. Rather, we'll ensure that all the 'create' commands go before the 'add' commands, directly while merging single set files, before 'ipset restore' is performed.

(In reply to Stefano Brivio from comment #11)
> Thanks for your patch. While it definitely fixes the issue, it doesn't scale
> too well for sets containing a huge number of entries, because of the added
> computational complexity given by sort(1) applied on all the entries.

I've noticed that ipset restore also takes a lot longer than before. Prior to the upgrade to 7.6 it took about 4s, now it takes 40s.

> I benchmarked a few different solutions, and ended up with something
> similar, that doesn't however require sorting all the entries. Rather, we'll
> ensure that all the 'create' commands go before the 'add' commands, directly
> while merging single set files, before 'ipset restore' is performed.

Would it be possible to attach the patch that fixes it that way to this bug report?

Mvg,
Rik

(In reply to Rik Theys from comment #12)
> Would it be possible to attach the patch that fixes it that way to this bug
> report?

The proposed solution would be something on the lines of:

diff --git a/ipset.start-stop b/ipset.start-stop
index 6c84bd3..61178c1 100644
--- a/ipset.start-stop
+++ b/ipset.start-stop
@@ -104,9 +104,16 @@ load() {
 	chmod 600 "${merged}"
 	set +f
 	if [ -d ${IPSET_DATA_DIR} ]; then
+		# Copy the first lines of each saved set first, as they create
+		# the sets, then the rest: list:set entries depend on other
+		# sets, so make sure they all get created first
 		for f in ${IPSET_DATA_DIR}/*; do
 			[ "${f}" = "${IPSET_DATA_DIR}/*" ] && break
-			cat "${f}" >> ${merged}
+			head -n1 "${f}" >> ${merged}
+		done
+		for f in ${IPSET_DATA_DIR}/*; do
+			[ "${f}" = "${IPSET_DATA_DIR}/*" ] && break
+			tail -n +2 "${f}" >> ${merged}
 		done
 	fi
 	set -f

Please mind that this is not tested and will not necessarily be the final fix.

MfG,
Stefano

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2019:2158