diff mbox series

net/tap: avoid using SIGIO

Message ID 20200714235810.5286-1-stephen@networkplumber.org (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/tap: avoid using SIGIO |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

Stephen Hemminger July 14, 2020, 11:58 p.m. UTC 
  SIGIO maybe used by application, instead choose another rt-signal.
Linux allows any signal to be used for signal based IO.
Search for an unused signal in the available rt-signal range.

Add more error checking for fcntl and signal handling.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
Resending original version, marked as PATCH now.

 drivers/net/tap/rte_eth_tap.c | 99 ++++++++++++++++++++++++-----------
 1 file changed, 67 insertions(+), 32 deletions(-)

Comments

Morten Brørup July 27, 2020, 1:19 p.m. UTC | #1 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, July 15, 2020 1:58 AM
> 
> SIGIO maybe used by application, instead choose another rt-signal.
> Linux allows any signal to be used for signal based IO.
> Search for an unused signal in the available rt-signal range.

Just an observation. Feel free to ignore at your convenience:

The problem is the same as for SIGIO if the application sets up its own signal handler after this, and uses some hardcoded rt-signal that happens to be the one found to be free.

Unless the application doesn't use a hardcoded rt-signal, but also searches for an unused one.

So perhaps the "search for unused rt-signal" should be exposed as a generic support function for the application (and this driver) to use.

> 
> Add more error checking for fcntl and signal handling.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> Resending original version, marked as PATCH now.
> 
>  drivers/net/tap/rte_eth_tap.c | 99 ++++++++++++++++++++++++-----------
>  1 file changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c
> index 339f24bf82f3..b19e26ba0e65 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -134,7 +134,7 @@ tun_alloc(struct pmd_internals *pmd, int
> is_keepalive)
>  #ifdef IFF_MULTI_QUEUE
>  	unsigned int features;
>  #endif
> -	int fd;
> +	int fd, signo, flags;
> 
>  	memset(&ifr, 0, sizeof(struct ifreq));
> 
> @@ -199,52 +199,87 @@ tun_alloc(struct pmd_internals *pmd, int
> is_keepalive)
>  		}
>  	}
> 
> +	flags = fcntl(fd, F_GETFL);
> +	if (flags == -1) {
> +		TAP_LOG(WARNING,
> +			"Unable to get %s current flags\n",
> +			ifr.ifr_name);
> +		goto error;
> +	}
> +
>  	/* Always set the file descriptor to non-blocking */
> -	if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
> +	flags |= O_NONBLOCK;
> +	if (fcntl(fd, F_SETFL, flags) < 0) {
>  		TAP_LOG(WARNING,
>  			"Unable to set %s to nonblocking: %s",
>  			ifr.ifr_name, strerror(errno));
>  		goto error;
>  	}
> 
> -	/* Set up trigger to optimize empty Rx bursts */
> -	errno = 0;
> -	do {
> +	/* Find a free realtime signal */
> +	for (signo = SIGRTMIN + 1; signo < SIGRTMAX; signo++) {
>  		struct sigaction sa;
> -		int flags = fcntl(fd, F_GETFL);
> 
> -		if (flags == -1 || sigaction(SIGIO, NULL, &sa) == -1)
> +		if (sigaction(signo, NULL, &sa) == -1) {
> +			TAP_LOG(WARNING,
> +				"Unable to get current rt-signal %d handler",
> +				signo);
> +			goto error;
> +		}
> +
> +		/* Already have the handler we want on this signal  */
> +		if (sa.sa_handler == tap_trigger_cb)
>  			break;
> -		if (sa.sa_handler != tap_trigger_cb) {
> -			/*
> -			 * Make sure SIGIO is not already taken. This is done
> -			 * as late as possible to leave the application a
> -			 * chance to set up its own signal handler first.
> -			 */
> -			if (sa.sa_handler != SIG_IGN &&
> -			    sa.sa_handler != SIG_DFL) {
> -				errno = EBUSY;
> -				break;
> -			}
> -			sa = (struct sigaction){
> -				.sa_flags = SA_RESTART,
> -				.sa_handler = tap_trigger_cb,
> -			};
> -			if (sigaction(SIGIO, &sa, NULL) == -1)
> -				break;
> +
> +		/* Is handler in use by application */
> +		if (sa.sa_handler != SIG_DFL) {
> +			TAP_LOG(DEBUG,
> +				"Skipping used rt-signal %d", signo);
> +			continue;
>  		}
> -		/* Enable SIGIO on file descriptor */
> -		fcntl(fd, F_SETFL, flags | O_ASYNC);
> -		fcntl(fd, F_SETOWN, getpid());
> -	} while (0);
> 
> -	if (errno) {
> +		sa = (struct sigaction) {
> +			.sa_flags = SA_RESTART,
> +			.sa_handler = tap_trigger_cb,
> +		};
> +
> +		if (sigaction(signo, &sa, NULL) == -1) {
> +			TAP_LOG(WARNING,
> +				"Unable to set rt-signal %d handler\n", signo);
> +			goto error;
> +		}
> +
> +		/* Found a good signal to use */
> +		TAP_LOG(DEBUG,
> +			"Using rt-signal %d", signo);
> +		break;
> +	}
> +
> +	if (signo == SIGRTMAX) {
> +		TAP_LOG(WARNING, "All rt-signals are in use\n");
> +
>  		/* Disable trigger globally in case of error */
>  		tap_trigger = 0;
> -		TAP_LOG(WARNING, "Rx trigger disabled: %s",
> -			strerror(errno));
> -	}
> +		TAP_LOG(NOTICE, "No Rx trigger signal available\n");
> +	} else {
> +		/* Enable signal on file descriptor */
> +		if (fcntl(fd, F_SETSIG, signo) < 0) {
> +			TAP_LOG(WARNING, "Unable to set signo %d for fd %d:
> %s",
> +				signo, fd, strerror(errno));
> +			goto error;
> +		}
> +		if (fcntl(fd, F_SETFL, flags | O_ASYNC) < 0) {
> +			TAP_LOG(WARNING, "Unable to set fcntl flags: %s",
> +				strerror(errno));
> +			goto error;
> +		}
> 
> +		if (fcntl(fd, F_SETOWN, getpid()) < 0) {
> +			TAP_LOG(WARNING, "Unable to set fcntl owner: %s",
> +				strerror(errno));
> +			goto error;
> +		}
> +	}
>  	return fd;
> 
>  error:
> --
> 2.27.0
>
Stephen Hemminger July 27, 2020, 7:44 p.m. UTC | #2 
On Mon, 27 Jul 2020 15:19:32 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Wednesday, July 15, 2020 1:58 AM
> > 
> > SIGIO maybe used by application, instead choose another rt-signal.
> > Linux allows any signal to be used for signal based IO.
> > Search for an unused signal in the available rt-signal range.  
> 
> Just an observation. Feel free to ignore at your convenience:
> 
> The problem is the same as for SIGIO if the application sets up its own signal handler after this, and uses some hardcoded rt-signal that happens to be the one found to be free.
> 
> Unless the application doesn't use a hardcoded rt-signal, but also searches for an unused one.
> 
> So perhaps the "search for unused rt-signal" should be exposed as a generic support function for the application (and this driver) to use.

There is no safe way to use a signal deep inside DPDK in a driver.

This is not the kind of thing that should be exposed to the application.

The algorithm for finding an RT signal conforms to the recommended policy on the signal(7)
manual page.

       programs should never refer to real-time signals using hard-
       coded numbers, but instead should always refer to real-time signals
       using the notation SIGRTMIN+n, and include suitable (run-time) checks
       that SIGRTMIN+n does not exceed SIGRTMAX.

The application should be following the proscribed policy on the man page.
If it doesn't it is broken.
Morten Brørup July 28, 2020, 10:48 a.m. UTC | #3 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Monday, July 27, 2020 9:44 PM
> 
> On Mon, 27 Jul 2020 15:19:32 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> > > Sent: Wednesday, July 15, 2020 1:58 AM
> > >
> > > SIGIO maybe used by application, instead choose another rt-signal.
> > > Linux allows any signal to be used for signal based IO.
> > > Search for an unused signal in the available rt-signal range.
> >
> > Just an observation. Feel free to ignore at your convenience:
> >
> > The problem is the same as for SIGIO if the application sets up its
> own signal handler after this, and uses some hardcoded rt-signal that
> happens to be the one found to be free.
> >
> > Unless the application doesn't use a hardcoded rt-signal, but also
> searches for an unused one.
> >
> > So perhaps the "search for unused rt-signal" should be exposed as a
> generic support function for the application (and this driver) to use.
> 
> There is no safe way to use a signal deep inside DPDK in a driver.
> 
> This is not the kind of thing that should be exposed to the
> application.
> 
> The algorithm for finding an RT signal conforms to the recommended
> policy on the signal(7)
> manual page.
> 
>        programs should never refer to real-time signals using hard-
>        coded numbers, but instead should always refer to real-time
> signals
>        using the notation SIGRTMIN+n, and include suitable (run-time)
> checks
>        that SIGRTMIN+n does not exceed SIGRTMAX.
> 
> The application should be following the proscribed policy on the man
> page.
> If it doesn't it is broken.

Great. That fully addresses my concern.

Acked-by: Morten Brørup <mb@smartsharesystems.com>
Ferruh Yigit Aug. 17, 2020, 3:26 p.m. UTC | #4 
On 7/28/2020 11:48 AM, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
>> Sent: Monday, July 27, 2020 9:44 PM
>>
>> On Mon, 27 Jul 2020 15:19:32 +0200
>> Morten Brørup <mb@smartsharesystems.com> wrote:
>>
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
>> Hemminger
>>>> Sent: Wednesday, July 15, 2020 1:58 AM
>>>>
>>>> SIGIO maybe used by application, instead choose another rt-signal.
>>>> Linux allows any signal to be used for signal based IO.
>>>> Search for an unused signal in the available rt-signal range.
>>>
>>> Just an observation. Feel free to ignore at your convenience:
>>>
>>> The problem is the same as for SIGIO if the application sets up its
>> own signal handler after this, and uses some hardcoded rt-signal that
>> happens to be the one found to be free.
>>>
>>> Unless the application doesn't use a hardcoded rt-signal, but also
>> searches for an unused one.
>>>
>>> So perhaps the "search for unused rt-signal" should be exposed as a
>> generic support function for the application (and this driver) to use.
>>
>> There is no safe way to use a signal deep inside DPDK in a driver.
>>
>> This is not the kind of thing that should be exposed to the
>> application.
>>
>> The algorithm for finding an RT signal conforms to the recommended
>> policy on the signal(7)
>> manual page.
>>
>>        programs should never refer to real-time signals using hard-
>>        coded numbers, but instead should always refer to real-time
>> signals
>>        using the notation SIGRTMIN+n, and include suitable (run-time)
>> checks
>>        that SIGRTMIN+n does not exceed SIGRTMAX.
>>
>> The application should be following the proscribed policy on the man
>> page.
>> If it doesn't it is broken.
> 
> Great. That fully addresses my concern.
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Ferruh Yigit Aug. 25, 2020, 7:33 p.m. UTC | #5 
On 8/17/2020 4:26 PM, Ferruh Yigit wrote:
> On 7/28/2020 11:48 AM, Morten Brørup wrote:
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
>>> Sent: Monday, July 27, 2020 9:44 PM
>>>
>>> On Mon, 27 Jul 2020 15:19:32 +0200
>>> Morten Brørup <mb@smartsharesystems.com> wrote:
>>>
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
>>> Hemminger
>>>>> Sent: Wednesday, July 15, 2020 1:58 AM
>>>>>
>>>>> SIGIO maybe used by application, instead choose another rt-signal.
>>>>> Linux allows any signal to be used for signal based IO.
>>>>> Search for an unused signal in the available rt-signal range.
>>>>
>>>> Just an observation. Feel free to ignore at your convenience:
>>>>
>>>> The problem is the same as for SIGIO if the application sets up its
>>> own signal handler after this, and uses some hardcoded rt-signal that
>>> happens to be the one found to be free.
>>>>
>>>> Unless the application doesn't use a hardcoded rt-signal, but also
>>> searches for an unused one.
>>>>
>>>> So perhaps the "search for unused rt-signal" should be exposed as a
>>> generic support function for the application (and this driver) to use.
>>>
>>> There is no safe way to use a signal deep inside DPDK in a driver.
>>>
>>> This is not the kind of thing that should be exposed to the
>>> application.
>>>
>>> The algorithm for finding an RT signal conforms to the recommended
>>> policy on the signal(7)
>>> manual page.
>>>
>>>        programs should never refer to real-time signals using hard-
>>>        coded numbers, but instead should always refer to real-time
>>> signals
>>>        using the notation SIGRTMIN+n, and include suitable (run-time)
>>> checks
>>>        that SIGRTMIN+n does not exceed SIGRTMAX.
>>>
>>> The application should be following the proscribed policy on the man
>>> page.
>>> If it doesn't it is broken.
>>
>> Great. That fully addresses my concern.
>>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/main, thanks.
diff mbox series

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 339f24bf82f3..b19e26ba0e65 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -134,7 +134,7 @@  tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 #ifdef IFF_MULTI_QUEUE
 	unsigned int features;
 #endif
-	int fd;
+	int fd, signo, flags;
 
 	memset(&ifr, 0, sizeof(struct ifreq));
 
@@ -199,52 +199,87 @@  tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 		}
 	}
 
+	flags = fcntl(fd, F_GETFL);
+	if (flags == -1) {
+		TAP_LOG(WARNING,
+			"Unable to get %s current flags\n",
+			ifr.ifr_name);
+		goto error;
+	}
+
 	/* Always set the file descriptor to non-blocking */
-	if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
+	flags |= O_NONBLOCK;
+	if (fcntl(fd, F_SETFL, flags) < 0) {
 		TAP_LOG(WARNING,
 			"Unable to set %s to nonblocking: %s",
 			ifr.ifr_name, strerror(errno));
 		goto error;
 	}
 
-	/* Set up trigger to optimize empty Rx bursts */
-	errno = 0;
-	do {
+	/* Find a free realtime signal */
+	for (signo = SIGRTMIN + 1; signo < SIGRTMAX; signo++) {
 		struct sigaction sa;
-		int flags = fcntl(fd, F_GETFL);
 
-		if (flags == -1 || sigaction(SIGIO, NULL, &sa) == -1)
+		if (sigaction(signo, NULL, &sa) == -1) {
+			TAP_LOG(WARNING,
+				"Unable to get current rt-signal %d handler",
+				signo);
+			goto error;
+		}
+
+		/* Already have the handler we want on this signal  */
+		if (sa.sa_handler == tap_trigger_cb)
 			break;
-		if (sa.sa_handler != tap_trigger_cb) {
-			/*
-			 * Make sure SIGIO is not already taken. This is done
-			 * as late as possible to leave the application a
-			 * chance to set up its own signal handler first.
-			 */
-			if (sa.sa_handler != SIG_IGN &&
-			    sa.sa_handler != SIG_DFL) {
-				errno = EBUSY;
-				break;
-			}
-			sa = (struct sigaction){
-				.sa_flags = SA_RESTART,
-				.sa_handler = tap_trigger_cb,
-			};
-			if (sigaction(SIGIO, &sa, NULL) == -1)
-				break;
+
+		/* Is handler in use by application */
+		if (sa.sa_handler != SIG_DFL) {
+			TAP_LOG(DEBUG,
+				"Skipping used rt-signal %d", signo);
+			continue;
 		}
-		/* Enable SIGIO on file descriptor */
-		fcntl(fd, F_SETFL, flags | O_ASYNC);
-		fcntl(fd, F_SETOWN, getpid());
-	} while (0);
 
-	if (errno) {
+		sa = (struct sigaction) {
+			.sa_flags = SA_RESTART,
+			.sa_handler = tap_trigger_cb,
+		};
+
+		if (sigaction(signo, &sa, NULL) == -1) {
+			TAP_LOG(WARNING,
+				"Unable to set rt-signal %d handler\n", signo);
+			goto error;
+		}
+
+		/* Found a good signal to use */
+		TAP_LOG(DEBUG,
+			"Using rt-signal %d", signo);
+		break;
+	}
+
+	if (signo == SIGRTMAX) {
+		TAP_LOG(WARNING, "All rt-signals are in use\n");
+
 		/* Disable trigger globally in case of error */
 		tap_trigger = 0;
-		TAP_LOG(WARNING, "Rx trigger disabled: %s",
-			strerror(errno));
-	}
+		TAP_LOG(NOTICE, "No Rx trigger signal available\n");
+	} else {
+		/* Enable signal on file descriptor */
+		if (fcntl(fd, F_SETSIG, signo) < 0) {
+			TAP_LOG(WARNING, "Unable to set signo %d for fd %d: %s",
+				signo, fd, strerror(errno));
+			goto error;
+		}
+		if (fcntl(fd, F_SETFL, flags | O_ASYNC) < 0) {
+			TAP_LOG(WARNING, "Unable to set fcntl flags: %s",
+				strerror(errno));
+			goto error;
+		}
 
+		if (fcntl(fd, F_SETOWN, getpid()) < 0) {
+			TAP_LOG(WARNING, "Unable to set fcntl owner: %s",
+				strerror(errno));
+			goto error;
+		}
+	}
 	return fd;
 
 error: