Paths

Table of Contentst

-
sys/netipsec/
-
netipsec/
-
ipsec_offload.c
-
key.c

Differential D46483

ipsec: Drain async ipsec_offload work when destroying a vnet
ClosedPublic
Actions

Authored by markj on Aug 30 2024, 12:54 AM.

Details

Reviewers

kib

Commits

rG01f43479b592: ipsec: Drain async ipsec_offload work when destroying a vnet
rGe196b12f4d4d: ipsec: Drain async ipsec_offload work when destroying a vnet

Summary

The ipsec_offload code in some cases releases object references in an
asynchronous context where it needs to set the current VNET. Make sure
that all such work completes before the VNET is actually destroyed,
otherwise a use-after-free is possible.

Reported by: KASAN
Fixes: ef2a572bf6bd ("ipsec_offload: kernel infrastructure")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Aug 30 2024, 12:54 AM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptAug 30 2024, 12:54 AM

markj requested review of this revision.Aug 30 2024, 12:54 AM

Harbormaster completed remote builds in B59238: Diff 142607.Aug 30 2024, 12:54 AM

kib accepted this revision.Aug 30 2024, 6:35 AM

This revision is now accepted and ready to land.Aug 30 2024, 6:35 AM

Closed by commit rGe196b12f4d4d: ipsec: Drain async ipsec_offload work when destroying a vnet (authored by markj). · Explain WhyAug 30 2024, 1:10 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rGe196b12f4d4d: ipsec: Drain async ipsec_offload work when destroying a vnet.

markj added a reverting change: rG28294dc92476: Revert "ipsec: Drain async ipsec_offload work when destroying a vnet".Aug 30 2024, 3:01 PM

This patch can cause hangs, since it's possible for key_vnet_destroy() to execute in taskqueue_thread's context.

To fix this, I wonder if ipsec_accel can use a dedicated taskqueue thread instead of taskqueue_thread.

In D46483#1059583, @markj wrote:

This patch can cause hangs, since it's possible for key_vnet_destroy() to execute in taskqueue_thread's context.

To fix this, I wonder if ipsec_accel can use a dedicated taskqueue thread instead of taskqueue_thread.

It can, the restriction is that all handling should occur in the same single-threaded context.
I did not allocated dedicated thread because the events like creation or destruction of SAs are very rare, and creation or destruction of SPs almost never happen. So I do not want to waste the whole thread for this.

Then, for me this feels more like a bug in taskqueue_drain_all(). What about D46489 instead?

markj added a commit: rG01f43479b592: ipsec: Drain async ipsec_offload work when destroying a vnet.Sep 4 2024, 2:29 PM

Revision Contents
Changeset List

Path

Size

sys/

netipsec/

ipsec_offload.c

2 lines

key.c

3 lines

Diff 142619

View Options

sys/netipsec/ipsec_offload.c

	.\" Copyright (c) 1990, 1991, 1993	.\" Copyright (c) 1990, 1991, 1993 The Regents of the University of California.
	.\" The Regents of the University of California. All rights reserved.	.\" Copyright (c) 2015 Mariusz Zaborski <oshogbo@FreeBSD.org>
		.\" All rights reserved.
	.\"	.\"
	.\" This code is derived from software contributed to Berkeley by	.\" This code is derived from software contributed to Berkeley by
	.\" Chris Torek and the American National Standards Committee X3,	.\" Chris Torek and the American National Standards Committee X3,
Context not available.
	.\" @(#)fclose.3 8.1 (Berkeley) 6/4/93	.\" @(#)fclose.3 8.1 (Berkeley) 6/4/93
	.\" $FreeBSD$	.\" $FreeBSD$
	.\"	.\"
	.Dd April 22, 2006	.Dd Jun 1, 2015
	.Dt FCLOSE 3	.Dt FCLOSE 3
	.Os	.Os
		wblockUnsubmitted Done Inline Actions No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up diffs. I'll try to include some context to make this easier.) 38 the file descriptor will be written to it. wblock: No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up…
	.Sh NAME	.Sh NAME
	.Nm fclose ,	.Nm fclose ,
		.Nm fdclose ,
		wblockUnsubmitted Done Inline Actions Typo: s/arguemnt/argument/ wblock: Typo: s/arguemnt/argument/
		wblockUnsubmitted Done Inline Actions Oops, I missed the other typo: s/diffrent/different/ wblock: Oops, I missed the other typo: s/diffrent/different/
	.Nm fcloseall	.Nm fcloseall
		wblockUnsubmitted Done Inline Actions Needs a comma after this "descriptor", where the pause is located: 42 If the stream does not have an associated file descriptor, wblock: Needs a comma after this "descriptor", where the pause is located: 42 If the stream does not…
	.Nd close a stream	.Nd close a stream
	.Sh LIBRARY	.Sh LIBRARY
Context not available.
	.In stdio.h	.In stdio.h
	.Ft int	.Ft int
	.Fn fclose "FILE *stream"	.Fn fclose "FILE *stream"
		.Ft int
		.Fn fdclose "FILE stream" "int fdp"
	.Ft void	.Ft void
	.Fn fcloseall void	.Fn fcloseall void
	.Sh DESCRIPTION	.Sh DESCRIPTION
Context not available.
	.Xr fflush 3 .	.Xr fflush 3 .
	.Pp	.Pp
	The	The
		.Fn fdclose
		function is equivalent to
		.Fn fclose
		except that it does not close the underlying file descriptor.
		If
		.Fa fdp
		is not
		jillesUnsubmitted Done Inline Actions Calling the argument fdp instead of fd may make this clearer. Try: If .Fa fd is not .Dv NULL , the file descriptor will be written to it. jilles: Calling the argument fdp instead of fd may make this clearer. Try: If .Fa fd is not .Dv NULL…
		.Dv NULL ,
		jillesUnsubmitted Done Inline Actions The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have an associated file descriptor", possibly with examples like fmemopen(), funopen() and open_memstream(). These words also occur under ERRORS below. If the stream is not open, the behaviour is not defined and need not be described in the man page. jilles: The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have…
		the file descriptor, will be written to it.
		If the
		.Fa fdp
		arguemnt will be diffrent then NULL the file descriptor will be returned in it,
		If the stream does not have an associated file descriptor
		wblockUnsubmitted Done Inline Actions Needs a comma after "descriptor". wblock: Needs a comma after "descriptor".
		.Fa fdp
		will be set to -1.
		This type of stream is created with functions such as
		wblockUnsubmitted Done Inline Actions "Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of stream", or maybe it does not even need to be said. "functions like" is confusing, probably should be "such as". Finally, switch to active rather than passive voice by changing "could" to "can" or "is": Streams are created with functions such as or This type of stream is created with functions such as or even This type of stream is created with wblock: "Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of…
		.Xr fmemopen 3 ,
		.Xr funopen 3 ,
		or
		.Xr open_memstream 3 .
		.Pp
		The
	.Fn fcloseall	.Fn fcloseall
	function calls	function calls
	.Fn fclose	.Fn fclose
	on all open streams.	on all open streams.
	.Sh RETURN VALUES	.Sh RETURN VALUES
	Upon successful completion 0 is returned.	.Fn fcloseall
		returns no value.
		.Pp
		wblockUnsubmitted Done Inline Actions Several things: Not necessary to use "the blah function". After the first time, just say the name of the function: "blah returns..." (no leading "The"). "returns no value" is somewhat ambiguous, could be misread to mean it returns null. "does not return a value" might be more clear. So: .Fn fcloseall does not return a value. wblock: Several things: 1. Not necessary to use "the blah function". After the first time, just say…
		Upon successful completion the
		.Fn fclose
		and
		.Fn fdclose
		wblockUnsubmitted Done Inline Actions Start the new sentence on a new line: 99 This is intentional. 100 It makes it easier to make sure programs written under wblock: Start the new sentence on a new line: 99 This is intentional. 100 It makes it easier to…
		functions return 0.
	Otherwise,	Otherwise,
	.Dv EOF	.Dv EOF
	is returned and the global variable	is returned and the global variable
	.Va errno	.Va errno
	is set to indicate the error.	is set to indicate the error.
	In either case no further access to the stream is possible.
	.Sh ERRORS	.Sh ERRORS
		jillesUnsubmitted Done Inline Actions This change seems unnecessary. It is still the case that there are two situations: success and error. jilles: This change seems unnecessary. It is still the case that there are two situations: success and…
	The	The
		.Fn fdclose
		function shall fail if:
		wblockUnsubmitted Done Inline Actions "Shall" implies a recommendation. Probably better as "will". wblock: "Shall" implies a recommendation. Probably better as "will".
		.Bl -tag -width Er
		.It Bq Er EOPNOTSUPP
		The stream does not have an associated file descriptor.
		.Pp
		The
	.Fn fclose	.Fn fclose
	function	and
	may also fail and set	.Fn fdclose
		functions may also fail and set
	.Va errno	.Va errno
	for any of the errors specified for the routines	for any of the errors specified for
	.Xr close 2
	or
	.Xr fflush 3 .	.Xr fflush 3 .
		.Pp
		jillesUnsubmitted Done Inline Actions errno should keep its .Va markup jilles: errno should keep its .Va markup
		The
		.Fn fclose
		function may also fail and set errno for any of the errors specified for
		.Xr close 2.
	.Sh NOTES	.Sh NOTES
	The	The
	.Fn fclose	.Fn fclose
	function	and
	does not handle NULL arguments; they will result in a segmentation	.Fn fdclose
	violation.	functions do not handle NULL arguments in the
	This is intentional - it makes it easier to make sure programs written	.Fa stream
	under	variable; this will result in a segmentation violation.
		This is intentional. It makes it easier to make sure programs written under
		wblockUnsubmitted Done Inline Actions (Missing an "it" after "makes"). Use a full stop rather than an emdash: This is intentional. It makes it easier to make sure programs written under wblock: (Missing an "it" after "makes"). Use a full stop rather than an emdash: This is intentional.
	.Fx	.Fx
	are bug free.	are bug free.
	This behaviour is an implementation detail, and programs should not	This behaviour is an implementation detail, and programs should not
Context not available.
	function	function
	conforms to	conforms to
	.St -isoC .	.St -isoC .
	.Pp	.Sh HISTORY
		jillesUnsubmitted Done Inline Actions This should be `.Sh HISTORY`. jilles: This should be `.Sh HISTORY`.
	The	The
	.Fn fcloseall	.Fn fcloseall
	function first appeared in	function first appeared in
	.Fx 7.0 .	.Fx 7.0 .
		.Pp
		The
		.Fn fdclose
		function first appeared in
		.Fx 11.0 .
Context not available.