[PATCH/RFC] mount: improve signal management when locking mtab.

[PATCH/RFC] mount: improve signal management when locking mtab.

[NeilBrown]

From: Neil Brown <neilb@suse.de>
Date: Wed, 22 Jun 2011 16:15:45 +1000
Subject: [PATCH] mount: improve signal management when locking mtab.

As mount.nfs can run setuid it must be careful about how the user can
interact with in.  In particular it needs to ensure it does not
respond badly to any signals that the user might be able to generate.

This is particularly an issue while updating /etc/mtab (when that is
not linked to /proc/mounts).  If the user can generate a signal which
kills mount.nfs while /etc/mtab is locked, then it will leave the file
locked, and could possibly corrupt mtab (particularly if 'ulimit 1'
was previously issued).

Currently lock_mtab does set some handlers for signals, but not
enough.  It arranges for every signal up to (but not including)
SIGCHLD to cause mount.nfs to unlock mdadm promptly exit ... even if
the default behaviour would be to ignore the signal.  SIGALRM is
handled specially, and signals after SIGCHLD are left with their
default behaviour.  This includes for example SIGXFSZ which can be
generated by the user running "ulimit 1".

So: change this so that some signals are left unchanged, SIGALRM is
handled as required, and all signals that the user can generate are
explicitly ignored.

The remainder still cause mount.nfs to print a message, unlock mtab, and exit.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 utils/mount/fstab.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
index a742e64..1fc9efe 100644
--- a/utils/mount/fstab.c
+++ b/utils/mount/fstab.c
@@ -331,16 +331,43 @@ lock_mtab (void) {
 		int sig = 0;
 		struct sigaction sa;
 
-		sa.sa_handler = handler;
 		sa.sa_flags = 0;
 		sigfillset (&sa.sa_mask);
   
-		while (sigismember (&sa.sa_mask, ++sig) != -1
-		       && sig != SIGCHLD) {
-			if (sig == SIGALRM)
+		while (sigismember (&sa.sa_mask, ++sig) != -1) {
+			switch(sig) {
+			case SIGCHLD:
+			case SIGKILL:
+			case SIGCONT:
+			case SIGSTOP:
+				/* The cannot be caught, or should not,
+				 * so don't even try.
+				 */
+				continue;
+			case SIGALRM:
 				sa.sa_handler = setlkw_timeout;
-			else
+				break;
+			case SIGHUP:
+			case SIGINT:
+			case SIGQUIT:
+			case SIGWINCH:
+			case SIGTSTP:
+			case SIGTTIN:
+			case SIGTTOU:
+			case SIGPIPE:
+			case SIGXFSZ:
+			case SIGXCPU:
+				/* non-priv user can cause these to be
+				 * generated, so ignore them.
+				 */
+				sa.sa_handler = SIG_IGN;
+				break;
+			default:
+				/* The rest should not be possible, so just
+				 * print a message and unlock mtab.
+				 */
 				sa.sa_handler = handler;
+			}
 			sigaction (sig, &sa, (struct sigaction *) 0);
 		}
 		signals_have_been_setup = 1;
-- 
1.7.3.4

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[Chuck Lever]

Since we are going to adopt libmount for mount.nfs in the near future, would it be better to update libmount instead?

On Jun 22, 2011, at 12:38 AM, NeilBrown wrote:

> 
> From: Neil Brown <neilb@suse.de>
> Date: Wed, 22 Jun 2011 16:15:45 +1000
> Subject: [PATCH] mount: improve signal management when locking mtab.
> 
> As mount.nfs can run setuid it must be careful about how the user can
> interact with in.  In particular it needs to ensure it does not
> respond badly to any signals that the user might be able to generate.
> 
> This is particularly an issue while updating /etc/mtab (when that is
> not linked to /proc/mounts).  If the user can generate a signal which
> kills mount.nfs while /etc/mtab is locked, then it will leave the file
> locked, and could possibly corrupt mtab (particularly if 'ulimit 1'
> was previously issued).
> 
> Currently lock_mtab does set some handlers for signals, but not
> enough.  It arranges for every signal up to (but not including)
> SIGCHLD to cause mount.nfs to unlock mdadm promptly exit ... even if
> the default behaviour would be to ignore the signal.  SIGALRM is
> handled specially, and signals after SIGCHLD are left with their
> default behaviour.  This includes for example SIGXFSZ which can be
> generated by the user running "ulimit 1".
> 
> So: change this so that some signals are left unchanged, SIGALRM is
> handled as required, and all signals that the user can generate are
> explicitly ignored.
> 
> The remainder still cause mount.nfs to print a message, unlock mtab, and exit.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> utils/mount/fstab.c |   37 ++++++++++++++++++++++++++++++++-----
> 1 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
> index a742e64..1fc9efe 100644
> --- a/utils/mount/fstab.c
> +++ b/utils/mount/fstab.c
> @@ -331,16 +331,43 @@ lock_mtab (void) {
> 		int sig = 0;
> 		struct sigaction sa;
> 
> -		sa.sa_handler = handler;
> 		sa.sa_flags = 0;
> 		sigfillset (&sa.sa_mask);
> 
> -		while (sigismember (&sa.sa_mask, ++sig) != -1
> -		       && sig != SIGCHLD) {
> -			if (sig == SIGALRM)
> +		while (sigismember (&sa.sa_mask, ++sig) != -1) {
> +			switch(sig) {
> +			case SIGCHLD:
> +			case SIGKILL:
> +			case SIGCONT:
> +			case SIGSTOP:
> +				/* The cannot be caught, or should not,
> +				 * so don't even try.
> +				 */
> +				continue;
> +			case SIGALRM:
> 				sa.sa_handler = setlkw_timeout;
> -			else
> +				break;
> +			case SIGHUP:
> +			case SIGINT:
> +			case SIGQUIT:
> +			case SIGWINCH:
> +			case SIGTSTP:
> +			case SIGTTIN:
> +			case SIGTTOU:
> +			case SIGPIPE:
> +			case SIGXFSZ:
> +			case SIGXCPU:
> +				/* non-priv user can cause these to be
> +				 * generated, so ignore them.
> +				 */
> +				sa.sa_handler = SIG_IGN;
> +				break;
> +			default:
> +				/* The rest should not be possible, so just
> +				 * print a message and unlock mtab.
> +				 */
> 				sa.sa_handler = handler;
> +			}
> 			sigaction (sig, &sa, (struct sigaction *) 0);
> 		}
> 		signals_have_been_setup = 1;
> -- 
> 1.7.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[NeilBrown]

On Wed, 22 Jun 2011 09:08:25 -0600 Chuck Lever <chuck.lever@oracle.com> wrote:

> Since we are going to adopt libmount for mount.nfs in the near future, would it be better to update libmount instead?

How near?

Libmount appears to get signal handling right already (though I haven't
tested it).  It blocks all signals rather than catching some of them.

So switching to libmount would a perfectly reasonably response.  However if
that is more than a few weeks away I think I would rather see this fixed up
anyway...

Thanks,
NeilBrown


> 
> On Jun 22, 2011, at 12:38 AM, NeilBrown wrote:
> 
> > 
> > From: Neil Brown <neilb@suse.de>
> > Date: Wed, 22 Jun 2011 16:15:45 +1000
> > Subject: [PATCH] mount: improve signal management when locking mtab.
> > 
> > As mount.nfs can run setuid it must be careful about how the user can
> > interact with in.  In particular it needs to ensure it does not
> > respond badly to any signals that the user might be able to generate.
> > 
> > This is particularly an issue while updating /etc/mtab (when that is
> > not linked to /proc/mounts).  If the user can generate a signal which
> > kills mount.nfs while /etc/mtab is locked, then it will leave the file
> > locked, and could possibly corrupt mtab (particularly if 'ulimit 1'
> > was previously issued).
> > 
> > Currently lock_mtab does set some handlers for signals, but not
> > enough.  It arranges for every signal up to (but not including)
> > SIGCHLD to cause mount.nfs to unlock mdadm promptly exit ... even if
> > the default behaviour would be to ignore the signal.  SIGALRM is
> > handled specially, and signals after SIGCHLD are left with their
> > default behaviour.  This includes for example SIGXFSZ which can be
> > generated by the user running "ulimit 1".
> > 
> > So: change this so that some signals are left unchanged, SIGALRM is
> > handled as required, and all signals that the user can generate are
> > explicitly ignored.
> > 
> > The remainder still cause mount.nfs to print a message, unlock mtab, and exit.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > utils/mount/fstab.c |   37 ++++++++++++++++++++++++++++++++-----
> > 1 files changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
> > index a742e64..1fc9efe 100644
> > --- a/utils/mount/fstab.c
> > +++ b/utils/mount/fstab.c
> > @@ -331,16 +331,43 @@ lock_mtab (void) {
> > 		int sig = 0;
> > 		struct sigaction sa;
> > 
> > -		sa.sa_handler = handler;
> > 		sa.sa_flags = 0;
> > 		sigfillset (&sa.sa_mask);
> > 
> > -		while (sigismember (&sa.sa_mask, ++sig) != -1
> > -		       && sig != SIGCHLD) {
> > -			if (sig == SIGALRM)
> > +		while (sigismember (&sa.sa_mask, ++sig) != -1) {
> > +			switch(sig) {
> > +			case SIGCHLD:
> > +			case SIGKILL:
> > +			case SIGCONT:
> > +			case SIGSTOP:
> > +				/* The cannot be caught, or should not,
> > +				 * so don't even try.
> > +				 */
> > +				continue;
> > +			case SIGALRM:
> > 				sa.sa_handler = setlkw_timeout;
> > -			else
> > +				break;
> > +			case SIGHUP:
> > +			case SIGINT:
> > +			case SIGQUIT:
> > +			case SIGWINCH:
> > +			case SIGTSTP:
> > +			case SIGTTIN:
> > +			case SIGTTOU:
> > +			case SIGPIPE:
> > +			case SIGXFSZ:
> > +			case SIGXCPU:
> > +				/* non-priv user can cause these to be
> > +				 * generated, so ignore them.
> > +				 */
> > +				sa.sa_handler = SIG_IGN;
> > +				break;
> > +			default:
> > +				/* The rest should not be possible, so just
> > +				 * print a message and unlock mtab.
> > +				 */
> > 				sa.sa_handler = handler;
> > +			}
> > 			sigaction (sig, &sa, (struct sigaction *) 0);
> > 		}
> > 		signals_have_been_setup = 1;
> > -- 
> > 1.7.3.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[Chuck Lever]

On Jun 22, 2011, at 5:20 PM, NeilBrown wrote:

> On Wed, 22 Jun 2011 09:08:25 -0600 Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> Since we are going to adopt libmount for mount.nfs in the near future, would it be better to update libmount instead?
> 
> How near?
> 
> Libmount appears to get signal handling right already (though I haven't
> tested it).  It blocks all signals rather than catching some of them.
> 
> So switching to libmount would a perfectly reasonably response.  However if
> that is more than a few weeks away I think I would rather see this fixed up
> anyway...

We have a libmount-based mount.nfs already in the nfs-utils tree, IIRC.  I don't think we yet have a generic plan for switching to installing that one by default.  Steve?

> Thanks,
> NeilBrown
> 
> 
>> 
>> On Jun 22, 2011, at 12:38 AM, NeilBrown wrote:
>> 
>>> 
>>> From: Neil Brown <neilb@suse.de>
>>> Date: Wed, 22 Jun 2011 16:15:45 +1000
>>> Subject: [PATCH] mount: improve signal management when locking mtab.
>>> 
>>> As mount.nfs can run setuid it must be careful about how the user can
>>> interact with in.  In particular it needs to ensure it does not
>>> respond badly to any signals that the user might be able to generate.
>>> 
>>> This is particularly an issue while updating /etc/mtab (when that is
>>> not linked to /proc/mounts).  If the user can generate a signal which
>>> kills mount.nfs while /etc/mtab is locked, then it will leave the file
>>> locked, and could possibly corrupt mtab (particularly if 'ulimit 1'
>>> was previously issued).
>>> 
>>> Currently lock_mtab does set some handlers for signals, but not
>>> enough.  It arranges for every signal up to (but not including)
>>> SIGCHLD to cause mount.nfs to unlock mdadm promptly exit ... even if
>>> the default behaviour would be to ignore the signal.  SIGALRM is
>>> handled specially, and signals after SIGCHLD are left with their
>>> default behaviour.  This includes for example SIGXFSZ which can be
>>> generated by the user running "ulimit 1".
>>> 
>>> So: change this so that some signals are left unchanged, SIGALRM is
>>> handled as required, and all signals that the user can generate are
>>> explicitly ignored.
>>> 
>>> The remainder still cause mount.nfs to print a message, unlock mtab, and exit.
>>> 
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>> utils/mount/fstab.c |   37 ++++++++++++++++++++++++++++++++-----
>>> 1 files changed, 32 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
>>> index a742e64..1fc9efe 100644
>>> --- a/utils/mount/fstab.c
>>> +++ b/utils/mount/fstab.c
>>> @@ -331,16 +331,43 @@ lock_mtab (void) {
>>> 		int sig = 0;
>>> 		struct sigaction sa;
>>> 
>>> -		sa.sa_handler = handler;
>>> 		sa.sa_flags = 0;
>>> 		sigfillset (&sa.sa_mask);
>>> 
>>> -		while (sigismember (&sa.sa_mask, ++sig) != -1
>>> -		       && sig != SIGCHLD) {
>>> -			if (sig == SIGALRM)
>>> +		while (sigismember (&sa.sa_mask, ++sig) != -1) {
>>> +			switch(sig) {
>>> +			case SIGCHLD:
>>> +			case SIGKILL:
>>> +			case SIGCONT:
>>> +			case SIGSTOP:
>>> +				/* The cannot be caught, or should not,
>>> +				 * so don't even try.
>>> +				 */
>>> +				continue;
>>> +			case SIGALRM:
>>> 				sa.sa_handler = setlkw_timeout;
>>> -			else
>>> +				break;
>>> +			case SIGHUP:
>>> +			case SIGINT:
>>> +			case SIGQUIT:
>>> +			case SIGWINCH:
>>> +			case SIGTSTP:
>>> +			case SIGTTIN:
>>> +			case SIGTTOU:
>>> +			case SIGPIPE:
>>> +			case SIGXFSZ:
>>> +			case SIGXCPU:
>>> +				/* non-priv user can cause these to be
>>> +				 * generated, so ignore them.
>>> +				 */
>>> +				sa.sa_handler = SIG_IGN;
>>> +				break;
>>> +			default:
>>> +				/* The rest should not be possible, so just
>>> +				 * print a message and unlock mtab.
>>> +				 */
>>> 				sa.sa_handler = handler;
>>> +			}
>>> 			sigaction (sig, &sa, (struct sigaction *) 0);
>>> 		}
>>> 		signals_have_been_setup = 1;
>>> -- 
>>> 1.7.3.4
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
> 

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[Steve Dickson]

On 06/23/2011 12:09 PM, Chuck Lever wrote:
> 
> On Jun 22, 2011, at 5:20 PM, NeilBrown wrote:
> 
>> On Wed, 22 Jun 2011 09:08:25 -0600 Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>> Since we are going to adopt libmount for mount.nfs in the near future, would it be better to update libmount instead?
>>
>> How near?
>>
>> Libmount appears to get signal handling right already (though I haven't
>> tested it).  It blocks all signals rather than catching some of them.
>>
>> So switching to libmount would a perfectly reasonably response.  However if
>> that is more than a few weeks away I think I would rather see this fixed up
>> anyway...
> 
> We have a libmount-based mount.nfs already in the nfs-utils tree, IIRC.  I don't think we yet have a generic plan for switching to installing that one by default.  Steve?
Well a while back I did add the libmount code along with --enable-libmount-mount 
configure flag. This flag has been enabled for a while now in the 
pre release of Fedora 16 so it will on in the release of f16. 

Plus I am looking to make a nfs-utils release... I'm in the process of
clean things up just to do that. So I would be willing to enable the libmount
code to be on by default. But I am concern not all distros do include
the libmount code... 

Some clarity... If the libmount code is enabled, this patch is not needed?

steved.

>> Thanks,
>> NeilBrown
>>
>>
>>>
>>> On Jun 22, 2011, at 12:38 AM, NeilBrown wrote:
>>>
>>>>
>>>> From: Neil Brown <neilb@suse.de>
>>>> Date: Wed, 22 Jun 2011 16:15:45 +1000
>>>> Subject: [PATCH] mount: improve signal management when locking mtab.
>>>>
>>>> As mount.nfs can run setuid it must be careful about how the user can
>>>> interact with in.  In particular it needs to ensure it does not
>>>> respond badly to any signals that the user might be able to generate.
>>>>
>>>> This is particularly an issue while updating /etc/mtab (when that is
>>>> not linked to /proc/mounts).  If the user can generate a signal which
>>>> kills mount.nfs while /etc/mtab is locked, then it will leave the file
>>>> locked, and could possibly corrupt mtab (particularly if 'ulimit 1'
>>>> was previously issued).
>>>>
>>>> Currently lock_mtab does set some handlers for signals, but not
>>>> enough.  It arranges for every signal up to (but not including)
>>>> SIGCHLD to cause mount.nfs to unlock mdadm promptly exit ... even if
>>>> the default behaviour would be to ignore the signal.  SIGALRM is
>>>> handled specially, and signals after SIGCHLD are left with their
>>>> default behaviour.  This includes for example SIGXFSZ which can be
>>>> generated by the user running "ulimit 1".
>>>>
>>>> So: change this so that some signals are left unchanged, SIGALRM is
>>>> handled as required, and all signals that the user can generate are
>>>> explicitly ignored.
>>>>
>>>> The remainder still cause mount.nfs to print a message, unlock mtab, and exit.
>>>>
>>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>> ---
>>>> utils/mount/fstab.c |   37 ++++++++++++++++++++++++++++++++-----
>>>> 1 files changed, 32 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
>>>> index a742e64..1fc9efe 100644
>>>> --- a/utils/mount/fstab.c
>>>> +++ b/utils/mount/fstab.c
>>>> @@ -331,16 +331,43 @@ lock_mtab (void) {
>>>> 		int sig = 0;
>>>> 		struct sigaction sa;
>>>>
>>>> -		sa.sa_handler = handler;
>>>> 		sa.sa_flags = 0;
>>>> 		sigfillset (&sa.sa_mask);
>>>>
>>>> -		while (sigismember (&sa.sa_mask, ++sig) != -1
>>>> -		       && sig != SIGCHLD) {
>>>> -			if (sig == SIGALRM)
>>>> +		while (sigismember (&sa.sa_mask, ++sig) != -1) {
>>>> +			switch(sig) {
>>>> +			case SIGCHLD:
>>>> +			case SIGKILL:
>>>> +			case SIGCONT:
>>>> +			case SIGSTOP:
>>>> +				/* The cannot be caught, or should not,
>>>> +				 * so don't even try.
>>>> +				 */
>>>> +				continue;
>>>> +			case SIGALRM:
>>>> 				sa.sa_handler = setlkw_timeout;
>>>> -			else
>>>> +				break;
>>>> +			case SIGHUP:
>>>> +			case SIGINT:
>>>> +			case SIGQUIT:
>>>> +			case SIGWINCH:
>>>> +			case SIGTSTP:
>>>> +			case SIGTTIN:
>>>> +			case SIGTTOU:
>>>> +			case SIGPIPE:
>>>> +			case SIGXFSZ:
>>>> +			case SIGXCPU:
>>>> +				/* non-priv user can cause these to be
>>>> +				 * generated, so ignore them.
>>>> +				 */
>>>> +				sa.sa_handler = SIG_IGN;
>>>> +				break;
>>>> +			default:
>>>> +				/* The rest should not be possible, so just
>>>> +				 * print a message and unlock mtab.
>>>> +				 */
>>>> 				sa.sa_handler = handler;
>>>> +			}
>>>> 			sigaction (sig, &sa, (struct sigaction *) 0);
>>>> 		}
>>>> 		signals_have_been_setup = 1;
>>>> -- 
>>>> 1.7.3.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> Chuck Lever
>>> chuck[dot]lever[at]oracle[dot]com
>>>
>>>
>>
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[NeilBrown]

On Mon, 27 Jun 2011 13:03:20 -0400 Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 06/23/2011 12:09 PM, Chuck Lever wrote:
> > 
> > On Jun 22, 2011, at 5:20 PM, NeilBrown wrote:
> > 
> >> On Wed, 22 Jun 2011 09:08:25 -0600 Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >>> Since we are going to adopt libmount for mount.nfs in the near future, would it be better to update libmount instead?
> >>
> >> How near?
> >>
> >> Libmount appears to get signal handling right already (though I haven't
> >> tested it).  It blocks all signals rather than catching some of them.
> >>
> >> So switching to libmount would a perfectly reasonably response.  However if
> >> that is more than a few weeks away I think I would rather see this fixed up
> >> anyway...
> > 
> > We have a libmount-based mount.nfs already in the nfs-utils tree, IIRC.  I don't think we yet have a generic plan for switching to installing that one by default.  Steve?
> Well a while back I did add the libmount code along with --enable-libmount-mount 
> configure flag. This flag has been enabled for a while now in the 
> pre release of Fedora 16 so it will on in the release of f16. 
> 
> Plus I am looking to make a nfs-utils release... I'm in the process of
> clean things up just to do that. So I would be willing to enable the libmount
> code to be on by default. But I am concern not all distros do include
> the libmount code... 
> 
> Some clarity... If the libmount code is enabled, this patch is not needed?
> 

No it is not ... however it would be nice if the affected code wasn't even
compiled in.

i.e. which a patch like this, it will not even compile the offending code if
libmount is selected.

NeilBrown


diff --git a/utils/mount/Makefile.am b/utils/mount/Makefile.am
index 056293c..7bc3e2b 100644
--- a/utils/mount/Makefile.am
+++ b/utils/mount/Makefile.am
@@ -9,11 +9,10 @@ man5_MANS	= nfs.man
 
 sbin_PROGRAMS	= mount.nfs
 EXTRA_DIST = nfsmount.x $(man8_MANS) $(man5_MANS)
-mount_common = error.c network.c fstab.c token.c \
+mount_common = error.c network.c token.c \
 		    parse_opt.c parse_dev.c \
 		    nfsmount.c nfs4mount.c stropts.c\
-		    nfsumount.c \
-		    mount_constants.h error.h network.h fstab.h token.h \
+		    mount_constants.h error.h network.h token.h \
 		    parse_opt.h parse_dev.h \
 		    nfs4_mount.h nfs_mount4.h stropts.h version.h \
 		    mount_config.h utils.c utils.h
@@ -33,7 +32,8 @@ if CONFIG_LIBMOUNT
 mount_nfs_SOURCES += mount_libmount.c
 mount_nfs_LDADD += $(LIBMOUNT)
 else
-mount_nfs_SOURCES += mount.c
+mount_nfs_SOURCES += mount.c fstab.c nfsumount.c fstab.h
+
 endif
 
 MAINTAINERCLEANFILES = Makefile.in

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[Steve Dickson]

On 06/27/2011 10:11 PM, NeilBrown wrote:
> On Mon, 27 Jun 2011 13:03:20 -0400 Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 06/23/2011 12:09 PM, Chuck Lever wrote:
>>>
>>> On Jun 22, 2011, at 5:20 PM, NeilBrown wrote:
>>>
>>>> On Wed, 22 Jun 2011 09:08:25 -0600 Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>>> Since we are going to adopt libmount for mount.nfs in the near future, would it be better to update libmount instead?
>>>>
>>>> How near?
>>>>
>>>> Libmount appears to get signal handling right already (though I haven't
>>>> tested it).  It blocks all signals rather than catching some of them.
>>>>
>>>> So switching to libmount would a perfectly reasonably response.  However if
>>>> that is more than a few weeks away I think I would rather see this fixed up
>>>> anyway...
>>>
>>> We have a libmount-based mount.nfs already in the nfs-utils tree, IIRC.  I don't think we yet have a generic plan for switching to installing that one by default.  Steve?
>> Well a while back I did add the libmount code along with --enable-libmount-mount 
>> configure flag. This flag has been enabled for a while now in the 
>> pre release of Fedora 16 so it will on in the release of f16. 
>>
>> Plus I am looking to make a nfs-utils release... I'm in the process of
>> clean things up just to do that. So I would be willing to enable the libmount
>> code to be on by default. But I am concern not all distros do include
>> the libmount code... 
>>
>> Some clarity... If the libmount code is enabled, this patch is not needed?
>>
> 
> No it is not ... however it would be nice if the affected code wasn't even
> compiled in.
> 
> i.e. which a patch like this, it will not even compile the offending code if
> libmount is selected.
> 
> NeilBrown
> 
> 
> diff --git a/utils/mount/Makefile.am b/utils/mount/Makefile.am
> index 056293c..7bc3e2b 100644
> --- a/utils/mount/Makefile.am
> +++ b/utils/mount/Makefile.am
> @@ -9,11 +9,10 @@ man5_MANS	= nfs.man
>  
>  sbin_PROGRAMS	= mount.nfs
>  EXTRA_DIST = nfsmount.x $(man8_MANS) $(man5_MANS)
> -mount_common = error.c network.c fstab.c token.c \
> +mount_common = error.c network.c token.c \
>  		    parse_opt.c parse_dev.c \
>  		    nfsmount.c nfs4mount.c stropts.c\
> -		    nfsumount.c \
> -		    mount_constants.h error.h network.h fstab.h token.h \
> +		    mount_constants.h error.h network.h token.h \
>  		    parse_opt.h parse_dev.h \
>  		    nfs4_mount.h nfs_mount4.h stropts.h version.h \
>  		    mount_config.h utils.c utils.h
> @@ -33,7 +32,8 @@ if CONFIG_LIBMOUNT
>  mount_nfs_SOURCES += mount_libmount.c
>  mount_nfs_LDADD += $(LIBMOUNT)
>  else
> -mount_nfs_SOURCES += mount.c
> +mount_nfs_SOURCES += mount.c fstab.c nfsumount.c fstab.h
> +
>  endif
>  
>  MAINTAINERCLEANFILES = Makefile.in
Good point.... I will take both patches...

steved.

Re: [PATCH/RFC] mount: improve signal management when locking mtab.

[Steve Dickson]

On 06/22/2011 02:38 AM, NeilBrown wrote:
> 
> From: Neil Brown <neilb@suse.de>
> Date: Wed, 22 Jun 2011 16:15:45 +1000
> Subject: [PATCH] mount: improve signal management when locking mtab.
> 
> As mount.nfs can run setuid it must be careful about how the user can
> interact with in.  In particular it needs to ensure it does not
> respond badly to any signals that the user might be able to generate.
> 
> This is particularly an issue while updating /etc/mtab (when that is
> not linked to /proc/mounts).  If the user can generate a signal which
> kills mount.nfs while /etc/mtab is locked, then it will leave the file
> locked, and could possibly corrupt mtab (particularly if 'ulimit 1'
> was previously issued).
> 
> Currently lock_mtab does set some handlers for signals, but not
> enough.  It arranges for every signal up to (but not including)
> SIGCHLD to cause mount.nfs to unlock mdadm promptly exit ... even if
> the default behaviour would be to ignore the signal.  SIGALRM is
> handled specially, and signals after SIGCHLD are left with their
> default behaviour.  This includes for example SIGXFSZ which can be
> generated by the user running "ulimit 1".
> 
> So: change this so that some signals are left unchanged, SIGALRM is
> handled as required, and all signals that the user can generate are
> explicitly ignored.
> 
> The remainder still cause mount.nfs to print a message, unlock mtab, and exit.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
Committed...

steved.

> ---
>  utils/mount/fstab.c |   37 ++++++++++++++++++++++++++++++++-----
>  1 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
> index a742e64..1fc9efe 100644
> --- a/utils/mount/fstab.c
> +++ b/utils/mount/fstab.c
> @@ -331,16 +331,43 @@ lock_mtab (void) {
>  		int sig = 0;
>  		struct sigaction sa;
>  
> -		sa.sa_handler = handler;
>  		sa.sa_flags = 0;
>  		sigfillset (&sa.sa_mask);
>    
> -		while (sigismember (&sa.sa_mask, ++sig) != -1
> -		       && sig != SIGCHLD) {
> -			if (sig == SIGALRM)
> +		while (sigismember (&sa.sa_mask, ++sig) != -1) {
> +			switch(sig) {
> +			case SIGCHLD:
> +			case SIGKILL:
> +			case SIGCONT:
> +			case SIGSTOP:
> +				/* The cannot be caught, or should not,
> +				 * so don't even try.
> +				 */
> +				continue;
> +			case SIGALRM:
>  				sa.sa_handler = setlkw_timeout;
> -			else
> +				break;
> +			case SIGHUP:
> +			case SIGINT:
> +			case SIGQUIT:
> +			case SIGWINCH:
> +			case SIGTSTP:
> +			case SIGTTIN:
> +			case SIGTTOU:
> +			case SIGPIPE:
> +			case SIGXFSZ:
> +			case SIGXCPU:
> +				/* non-priv user can cause these to be
> +				 * generated, so ignore them.
> +				 */
> +				sa.sa_handler = SIG_IGN;
> +				break;
> +			default:
> +				/* The rest should not be possible, so just
> +				 * print a message and unlock mtab.
> +				 */
>  				sa.sa_handler = handler;
> +			}
>  			sigaction (sig, &sa, (struct sigaction *) 0);
>  		}
>  		signals_have_been_setup = 1;