[PATCH] Remove file system sharemode before calling unlink

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Remove file system sharemode before calling unlink

Samba - samba-technical mailing list
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove file system sharemode before calling unlink

Samba - samba-technical mailing list
On Wed, Jan 10, 2018 at 04:38:29PM -0700, Christof Schmitt via samba-technical wrote:

> From 05c37256483d961a4448239292022ec38d8ff188 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <[hidden email]>
> Date: Wed, 10 Jan 2018 15:56:08 -0700
> Subject: [PATCH] Remove file system sharemode before calling unlink
>
> GPFS implements the DENY_DELETE sharemode, which prevents unlink() from
> deleting the file.. This causes the problem that deleting a file through
> "delete on close" fails, as the code in close.c first calls unlink() and
> only later removes the file system sharemode.
>
> Fix this by removing the file system sharemode before calling unlink().

Hmmm. This opens up a race that could cause the unlink
to then fail, if a non-smbd process (i.e. one that ignores
the tdb share mode lock) opens the path and sets a sharemode
between the SMB_VFS_KERNEL_FLOCK() and SMB_VFS_UNLINK()
calls, but I guess failing the unlink in that case is
also the right thing to do.

RB+ and I'll push if I can ever get an autobuild working
again :-).

Jeremy.

> Signed-off-by: Christof Schmitt <[hidden email]>
> ---
>  source3/smbd/close.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/source3/smbd/close.c b/source3/smbd/close.c
> index 095feec..2f6cc4f 100644
> --- a/source3/smbd/close.c
> +++ b/source3/smbd/close.c
> @@ -446,6 +446,22 @@ static NTSTATUS close_remove_share_mode(files_struct *fsp,
>   }
>   }
>  
> + if (fsp->kernel_share_modes_taken) {
> + int ret_flock;
> +
> + /*
> + * A file system sharemode could block the unlink;
> + * remove filesystem sharemodes first.
> + */
> + ret_flock = SMB_VFS_KERNEL_FLOCK(fsp, 0, 0);
> + if (ret_flock == -1) {
> + DBG_INFO("removing kernel flock for %s failed: %s\n",
> +  fsp_str_dbg(fsp), strerror(errno));
> + }
> +
> + fsp->kernel_share_modes_taken = false;
> + }
> +
>  
>   if (SMB_VFS_UNLINK(conn, fsp->fsp_name) != 0) {
>   /*
> --
> 1.8.3.1
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove file system sharemode before calling unlink

Samba - samba-technical mailing list
On Thu, Jan 11, 2018 at 03:34:47PM -0800, Jeremy Allison wrote:

> On Wed, Jan 10, 2018 at 04:38:29PM -0700, Christof Schmitt via samba-technical wrote:
> > From 05c37256483d961a4448239292022ec38d8ff188 Mon Sep 17 00:00:00 2001
> > From: Christof Schmitt <[hidden email]>
> > Date: Wed, 10 Jan 2018 15:56:08 -0700
> > Subject: [PATCH] Remove file system sharemode before calling unlink
> >
> > GPFS implements the DENY_DELETE sharemode, which prevents unlink() from
> > deleting the file.. This causes the problem that deleting a file through
> > "delete on close" fails, as the code in close.c first calls unlink() and
> > only later removes the file system sharemode.
> >
> > Fix this by removing the file system sharemode before calling unlink().
>
> Hmmm. This opens up a race that could cause the unlink
> to then fail, if a non-smbd process (i.e. one that ignores
> the tdb share mode lock) opens the path and sets a sharemode
> between the SMB_VFS_KERNEL_FLOCK() and SMB_VFS_UNLINK()
> calls, but I guess failing the unlink in that case is
> also the right thing to do.

Yes. The problem is that unlink() is not tied to the current file
descriptor, so the file system has to treat the unlink() as a sharemode
conflict.  There is nothing like a funlink() call to get around this...

> RB+ and I'll push if I can ever get an autobuild working
> again :-).

Thank you.

Christof

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove file system sharemode before calling unlink

Samba - samba-technical mailing list
On Fri, Jan 12, 2018 at 10:38:50AM -0700, Christof Schmitt wrote:

> On Thu, Jan 11, 2018 at 03:34:47PM -0800, Jeremy Allison wrote:
> > On Wed, Jan 10, 2018 at 04:38:29PM -0700, Christof Schmitt via samba-technical wrote:
> > > From 05c37256483d961a4448239292022ec38d8ff188 Mon Sep 17 00:00:00 2001
> > > From: Christof Schmitt <[hidden email]>
> > > Date: Wed, 10 Jan 2018 15:56:08 -0700
> > > Subject: [PATCH] Remove file system sharemode before calling unlink
> > >
> > > GPFS implements the DENY_DELETE sharemode, which prevents unlink() from
> > > deleting the file.. This causes the problem that deleting a file through
> > > "delete on close" fails, as the code in close.c first calls unlink() and
> > > only later removes the file system sharemode.
> > >
> > > Fix this by removing the file system sharemode before calling unlink().
> >
> > Hmmm. This opens up a race that could cause the unlink
> > to then fail, if a non-smbd process (i.e. one that ignores
> > the tdb share mode lock) opens the path and sets a sharemode
> > between the SMB_VFS_KERNEL_FLOCK() and SMB_VFS_UNLINK()
> > calls, but I guess failing the unlink in that case is
> > also the right thing to do.
>
> Yes. The problem is that unlink() is not tied to the current file
> descriptor, so the file system has to treat the unlink() as a sharemode
> conflict.  There is nothing like a funlink() call to get around this...
>
> > RB+ and I'll push if I can ever get an autobuild working
> > again :-).
>
> Thank you.

I just noticed that we are past the 4.8 branch and this needs to be
fixed in 4.8. Pleasd push the attached patch with a bugzilla link for the
backport.

Christof

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove file system sharemode before calling unlink

Samba - samba-technical mailing list
On Fri, Jan 12, 2018 at 11:23:25AM -0700, Christof Schmitt wrote:

> On Fri, Jan 12, 2018 at 10:38:50AM -0700, Christof Schmitt wrote:
> > On Thu, Jan 11, 2018 at 03:34:47PM -0800, Jeremy Allison wrote:
> > > On Wed, Jan 10, 2018 at 04:38:29PM -0700, Christof Schmitt via samba-technical wrote:
> > > > From 05c37256483d961a4448239292022ec38d8ff188 Mon Sep 17 00:00:00 2001
> > > > From: Christof Schmitt <[hidden email]>
> > > > Date: Wed, 10 Jan 2018 15:56:08 -0700
> > > > Subject: [PATCH] Remove file system sharemode before calling unlink
> > > >
> > > > GPFS implements the DENY_DELETE sharemode, which prevents unlink() from
> > > > deleting the file.. This causes the problem that deleting a file through
> > > > "delete on close" fails, as the code in close.c first calls unlink() and
> > > > only later removes the file system sharemode.
> > > >
> > > > Fix this by removing the file system sharemode before calling unlink().
> > >
> > > Hmmm. This opens up a race that could cause the unlink
> > > to then fail, if a non-smbd process (i.e. one that ignores
> > > the tdb share mode lock) opens the path and sets a sharemode
> > > between the SMB_VFS_KERNEL_FLOCK() and SMB_VFS_UNLINK()
> > > calls, but I guess failing the unlink in that case is
> > > also the right thing to do.
> >
> > Yes. The problem is that unlink() is not tied to the current file
> > descriptor, so the file system has to treat the unlink() as a sharemode
> > conflict.  There is nothing like a funlink() call to get around this...
> >
> > > RB+ and I'll push if I can ever get an autobuild working
> > > again :-).
> >
> > Thank you.
>
> I just noticed that we are past the 4.8 branch and this needs to be
> fixed in 4.8. Pleasd push the attached patch with a bugzilla link for the
> backport.

Will push + bugid in commit message once autobuild is working
again.