RFE: add transaction support to the libseccomp API #415

pcmoore · 2023-09-21T17:03:54Z

This PR contains two patches, the first fixes an existing problem with transaction management and the second adds a new transaction API to libseccomp.

@drakenclimber when you have the chance please take a look and let me know what you think, especially regarding the API as that is difficult/impossible to change later. If everything looks okay, I'll submit another PR with the "src/db.c" fix for the release-2.5 branch,

drakenclimber

Thanks for the patchset, @pcmoore. I think the user-facing API is good. I do have a few questions here and there but nothing earth shattering.

drakenclimber · 2023-09-27T21:08:53Z

src/db.c

@@ -2559,8 +2566,9 @@ void db_col_transaction_commit(struct db_filter_col *col)
 	if (snap->shadow) {
 		/* leave the shadow intact, but drop the next snapshot */


Is this block of code accessible? I hacked around for just a bit and was unable to hit it.

snap->shadow is effectively always set to false at the start of db_col_transaction_start(). snap->shadow is only set true at the end of db_col_transaction_commit() - approximately 100 lines after this. Is there a code path where snap->shadow is true at this point?

Unfortunately, it looks like I never responded back when I "fixed" this, so I'm sorta going by memory/comments/self-code-review and I believe the answer that yes, you can hit this code when you start nesting transactions.

drakenclimber · 2023-09-27T21:12:44Z

src/db.c

-			_db_snap_release(snap->next);
+			struct db_filter_snap *tmp = snap->next;
+			snap->next = tmp->next;
+			_db_snap_release(tmp);
 		}
 		return;
 	}


(This comment is for a couple lines down, but Github won't let me comment there. Specifically this line: https://github.com/pcmoore/misc-libseccomp/blob/working-transactions/src/db.c#L2589)

Since we had issues handling aborts, I wrote a quick test for this large block of code where the col's filter count didn't match the snap's filter count.

Here's the test I threw together:
drakenclimber@3814774

And here's the code coverage (prior to this test these lines weren't being hit):
https://coveralls.io/builds/62918086/source?filename=src%2Fdb.c#L2589

Interestingly this leads to a valgrind failure, but I haven't looked into it yet.

Thanks for the test! I've included it in my dev branch (which I'm going to refresh here in a few minutes) and used it to fix a number of bugs.

drakenclimber · 2023-09-27T21:14:03Z

doc/man/man3/seccomp_transaction_start.3

+into the kernel can not be modified, only new seccomp filters can be added on
+top of the existing loaded filter stack.
+.P
+Finishing, or committing, a transaction is optional, although it is encouraged.


I like this code block as it clearly explains why a user would be interested in this feature

As you have outlined in issue #181, this should greatly help in testing transactions 👍. Do you have a use case in mind for libseccomp's users (like systemd, etc.)?

No one particular use case in mind, I'll leave that up to more clever people than I :)

drakenclimber · 2023-09-27T21:19:56Z

include/seccomp.h.in

+ * This function rejects the current seccomp filter transaction.
+ *
+ */
+void seccomp_transaction_reject(const scmp_filter_ctx ctx);


Would we ever want a return value here? I can't think of one, but as you mentioned in the cover letter, the API is set in stone once we release this :)

I'm struggling to think of one too, what do you think, should we return an int?

drakenclimber · 2023-09-27T21:24:08Z

src/db.h

@@ -145,6 +145,7 @@ struct db_filter_snap {
 	struct db_filter **filters;
 	unsigned int filter_cnt;
 	bool shadow;
+	bool user;


I'm thinking a bool should meet our needs initially, but I'm wondering if long-term we'll want to switch to a transaction number (with a special bit for user-initiated transactions). I'm afraid the bool check of if snap->user != user may not be powerful enough someday.

The good thing is none of this exposed by the API, so we could change it later.

Agree, let's leave it as a bool now, mostly for simplicity; we can change it later if needed.

drakenclimber · 2023-10-18T20:26:10Z

Valgrind output of the test I added:

==1041378== Memcheck, a memory error detector
==1041378== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==1041378== Using Valgrind-3.21.0 and LibVEX; rerun with -h for copyright info
==1041378== Command: ./62-sim-arch_transactions -b
==1041378==
--1041378-- WARNING: unhandled amd64-linux syscall: 317
--1041378-- You may be able to write your own handler.
--1041378-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
--1041378-- Nevertheless we consider this a bug.  Please report
--1041378-- it at http://valgrind.org/support/bug_reports.html.
==1041378== Invalid read of size 8
==1041378==    at 0x406A09: db_col_transaction_commit (db.c:2616)
==1041378==    by 0x402930: seccomp_transaction_commit (api.c:831)
==1041378==    by 0x401375: main (62-sim-arch_transactions.c:92)
==1041378==  Address 0x4a4e9c0 is 0 bytes after a block of size 16 alloc'd
==1041378==    at 0x484782C: calloc (vg_replace_malloc.c:1554)
==1041378==    by 0x4064EB: db_col_transaction_start (db.c:2451)
==1041378==    by 0x4028D9: seccomp_transaction_start (api.c:806)
==1041378==    by 0x401345: main (62-sim-arch_transactions.c:85)
==1041378==
==1041378==
==1041378== HEAP SUMMARY:
==1041378==     in use at exit: 312 bytes in 3 blocks
==1041378==   total heap usage: 102 allocs, 99 frees, 9,864 bytes allocated
==1041378==
==1041378== 312 (32 direct, 280 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==1041378==    at 0x484782C: calloc (vg_replace_malloc.c:1554)
==1041378==    by 0x406543: _db_init (db.c:873)
==1041378==    by 0x406543: db_col_transaction_start (db.c:2465)
==1041378==    by 0x4028D9: seccomp_transaction_start (api.c:806)
==1041378==    by 0x401345: main (62-sim-arch_transactions.c:85)
==1041378==
==1041378== LEAK SUMMARY:
==1041378==    definitely lost: 32 bytes in 1 blocks
==1041378==    indirectly lost: 280 bytes in 2 blocks
==1041378==      possibly lost: 0 bytes in 0 blocks
==1041378==    still reachable: 0 bytes in 0 blocks
==1041378==         suppressed: 0 bytes in 0 blocks
==1041378==
==1041378== For lists of detected and suppressed errors, rerun with: -s
==1041378== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

drakenclimber · 2023-10-18T20:31:56Z

Well unsurprisingly these are the lines that valgrind is angry about:

		/* NOTE: while we release the filters we no longer need, we
		 *       don't bother to resize the filter array, we just
		 *       adjust the filter counter, this *should* be harmless
		 *       at the cost of a not reaping all the memory possible */

		do {
			_db_release(snap->filters[snap->filter_cnt--]);
		} while (snap->filter_cnt > col->filter_cnt);

drakenclimber · 2023-10-18T21:59:49Z

As expected, resizing the snap filter array appeases valgrind. I feel like it's a worthwhile change since this is likely an infrequent operation (and perhaps only in an error path). I think proper memory management may outweigh speed in this case.

Also, I don't claim to like my implementation - it was just a proof of concept to verify the root cause :).

@@ -2612,8 +2613,17 @@ void db_col_transaction_commit(struct db_filter_col *col, bool user)
                 *       adjust the filter counter, this *should* be harmless
                 *       at the cost of a not reaping all the memory possible */
                do {
+                       struct db_filter **dbs;
+
                        _db_release(snap->filters[snap->filter_cnt--]);
+
+                       dbs = realloc(snap->filters,
+                                     sizeof(struct db_filter *) * snap->filter_cnt);
+                       if (dbs != NULL)
+                               snap->filters = dbs;
                } while (snap->filter_cnt > col->filter_cnt);
        }

coveralls · 2024-04-08T16:16:01Z

coverage: 90.252% (+0.8%) from 89.454%
when pulling 893e70d on pcmoore:working-transactions
into 2847f10 on seccomp:main.

drakenclimber · 2024-04-13T21:41:16Z

I have a rough schedule the next few days, but I should have time at the end of the week to check this (and the other libseccomp patches) out later in the week.

pcmoore · 2024-04-14T14:37:48Z

I have a rough schedule the next few days, but I should have time at the end of the week to check this (and the other libseccomp patches) out later in the week.

Hi Tom, no immediate rush, but when you have the time it would be great to look over the other PRs; if you don't have time to merge them - assuming they look good to you - just leave your ACK and I'll take care of them.

As far as this PR is concerned, don't spend your time reviewing it just yet, I want to finish up a few things and make sure everything looks good on my end first; I'll leave a note when it is ready for review. Thanks for the previous review and feedback, it's been very helpful.

drakenclimber · 2024-04-15T13:14:08Z

I have a rough schedule the next few days, but I should have time at the end of the week to check this (and the other libseccomp patches) out later in the week.

Hi Tom, no immediate rush, but when you have the time it would be great to look over the other PRs; if you don't have time to merge them - assuming they look good to you - just leave your ACK and I'll take care of them.

As far as this PR is concerned, don't spend your time reviewing it just yet, I want to finish up a few things and make sure everything looks good on my end first; I'll leave a note when it is ready for review. Thanks for the previous review and feedback, it's been very helpful.

Sounds good. I'll focus on the other PRs this week. Let me know when this one is ready, and I'll start looking at it again.

drakenclimber · 2024-08-15T15:29:42Z

@pcmoore, I admit I haven't paid attention to this pull request lately. Do you want me to review, or are you still adding some more changes? Thanks!

pcmoore · 2024-09-05T18:19:34Z

I still need to give it a once over, but thanks for checking :)

It turns out we don't properly handle transaction aborts as we should, this likely broke when we implemented the shadow snapshots but as the transaction concept was not exported via the API, and callers most likely abandoned the filter on error this went unnoticed. This patch ensures that transaction aborts are handled properly by correctly managing the filter's transaction stack. Signed-off-by: Paul Moore <[email protected]>

Fix an off-by-one error that was causing us to leak a db_filter struct at transaction commit time when we removed an arch/ABI in a transaction. Reported-by: Tom Hromatka <[email protected]> Signed-off-by: Paul Moore <[email protected]>

While libseccomp has internally has transaction support for some time now, it hasn't been accessible to callers through the libseccomp API. This patch adds a transaction API as well as supporting documentation and a new unit regression test. int seccomp_transaction_start(const scmp_filter_ctx ctx) int seccomp_transaction_commit(const scmp_filter_ctx ctx) void seccomp_transaction_reject(const scmp_filter_ctx ctx) Signed-off-by: Paul Moore <[email protected]>

Add a test to verify the logic at the end of db_col_transaction_commit() properly copies and releases the snapshots from the filter when the filter length doesn't match the snapshot length. Signed-off-by: Tom Hromatka <[email protected]> [PM: subj tweak] Signed-off-by: Paul Moore <[email protected]>

pcmoore · 2024-09-09T23:30:48Z

Okay @drakenclimber, I think this is ready for a closer review when you have the time.

pcmoore added enhancement pending/review labels Sep 21, 2023

pcmoore added this to the v2.6.0 milestone Sep 21, 2023

pcmoore requested a review from drakenclimber September 21, 2023 17:03

pcmoore self-assigned this Sep 21, 2023

pcmoore mentioned this pull request Sep 21, 2023

RFE: expose the transaction mechanism for use by applications #181

Open

pcmoore linked an issue Sep 21, 2023 that may be closed by this pull request

RFE: expose the transaction mechanism for use by applications #181

Open

rusty-snake mentioned this pull request Sep 23, 2023

Upcomming libseccomp changes libseccomp-rs/libseccomp-rs#142

Open

19 tasks

drakenclimber reviewed Sep 27, 2023

View reviewed changes

pcmoore added pending/revision and removed pending/review labels Nov 30, 2023

pcmoore force-pushed the working-transactions branch from 6adf397 to a3a7056 Compare April 8, 2024 16:12

pcmoore force-pushed the working-transactions branch 2 times, most recently from 283f792 to 0deb5a2 Compare April 12, 2024 20:31

pcmoore marked this pull request as draft April 14, 2024 14:37

pcmoore and others added 4 commits September 9, 2024 19:08

pcmoore force-pushed the working-transactions branch from 0deb5a2 to 893e70d Compare September 9, 2024 23:29

pcmoore marked this pull request as ready for review September 9, 2024 23:30

pcmoore added pending/review and removed pending/revision labels Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFE: add transaction support to the libseccomp API #415

RFE: add transaction support to the libseccomp API #415

pcmoore commented Sep 21, 2023

drakenclimber left a comment

drakenclimber Sep 27, 2023

pcmoore Sep 9, 2024

drakenclimber Sep 27, 2023 •

edited

Loading

pcmoore Sep 9, 2024

drakenclimber Sep 27, 2023

drakenclimber Sep 27, 2023

pcmoore Sep 9, 2024

drakenclimber Sep 27, 2023

pcmoore Sep 9, 2024

drakenclimber Sep 27, 2023

pcmoore Sep 9, 2024

drakenclimber commented Oct 18, 2023

drakenclimber commented Oct 18, 2023 •

edited

Loading

drakenclimber commented Oct 18, 2023 •

edited

Loading

coveralls commented Apr 8, 2024 •

edited

Loading

drakenclimber commented Apr 13, 2024

pcmoore commented Apr 14, 2024

drakenclimber commented Apr 15, 2024

drakenclimber commented Aug 15, 2024

pcmoore commented Sep 5, 2024

pcmoore commented Sep 9, 2024

		@@ -2559,8 +2566,9 @@ void db_col_transaction_commit(struct db_filter_col *col)
		if (snap->shadow) {
		/* leave the shadow intact, but drop the next snapshot */

RFE: add transaction support to the libseccomp API #415

Are you sure you want to change the base?

RFE: add transaction support to the libseccomp API #415

Conversation

pcmoore commented Sep 21, 2023

drakenclimber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drakenclimber Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drakenclimber commented Oct 18, 2023

drakenclimber commented Oct 18, 2023 • edited Loading

drakenclimber commented Oct 18, 2023 • edited Loading

coveralls commented Apr 8, 2024 • edited Loading

drakenclimber commented Apr 13, 2024

pcmoore commented Apr 14, 2024

drakenclimber commented Apr 15, 2024

drakenclimber commented Aug 15, 2024

pcmoore commented Sep 5, 2024

pcmoore commented Sep 9, 2024

drakenclimber Sep 27, 2023 •

edited

Loading

drakenclimber commented Oct 18, 2023 •

edited

Loading

drakenclimber commented Oct 18, 2023 •

edited

Loading

coveralls commented Apr 8, 2024 •

edited

Loading