[PATCH]: s3: smbd: Fix delete-on-close after smb2_find

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

[PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
Hi!

On a customer system I came recently across the following Samba panic:

[2017/11/03 14:27:35.930242,  0, pid=446, effective(12000500, 12000513),
real(12000500, 0)] ../source3/lib/util.c:791(smb_panic_s3)
    PANIC (pid 446): assert failed: dirp->fsp->dptr->dir_hnd == dirp
[2017/11/03 14:27:35.930722,  0, pid=446, effective(12000500, 12000513),
real(12000500, 0)] ../source3/lib/util.c:902(log_stack_trace)
    BACKTRACE: 27 stack frames:
     #0 /usr/lpp/mmfs/lib64/libsmbconf.so.0(log_stack_trace+0x1a)
[0x7f3a05013e7a]
     #1 /usr/lpp/mmfs/lib64/libsmbconf.so.0(smb_panic_s3+0x20)
[0x7f3a05013f50]
     #2 /usr/lpp/mmfs/lib64/libsamba-util.so.0(smb_panic+0x2f)
[0x7f3a07919bdf]
     #3 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(+0xbb127)
[0x7f3a07461127]
     #4 /usr/lpp/mmfs/lib64/samba/libtalloc.so.2(_talloc_free+0x440)
[0x7f3a06d8fed0]
     #5
/usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(can_delete_directory_fsp+0x137)
[0x7f3a07464357]
     #6
/usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(can_set_delete_on_close+0x168)
[0x7f3a074e85b8]
     #7 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(+0xf4b73)
[0x7f3a0749ab73]
     #8
/usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_do_setfilepathinfo+0x169b)
[0x7f3a074ab0ab]
     #9
/usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_smb2_request_process_setinfo+0x630)
[0x7f3a07504c30]
     #10
/usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_smb2_request_dispatch+0xcb5)
[0x7f3a074ecb05]
     #11 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(+0x148f22)
[0x7f3a074eef22]
     #12 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x9c9b) [0x7f3a06984c9b]
     #13 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8167) [0x7f3a06983167]
     #14
/usr/lpp/mmfs/lib64/samba/libtevent.so.0(_tevent_loop_once+0x8d)
[0x7f3a0697f31d]
     #15
/usr/lpp/mmfs/lib64/samba/libtevent.so.0(tevent_common_loop_wait+0x1b)
[0x7f3a0697f4bb]
     #16 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8107) [0x7f3a06983107]
     #17
/usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_process+0x721)
[0x7f3a074db991]
     #18 /usr/lpp/mmfs/bin/smbd(+0xb588) [0x7f3a07fbc588]
     #19 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x9c9b) [0x7f3a06984c9b]
     #20 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8167) [0x7f3a06983167]
     #21
/usr/lpp/mmfs/lib64/samba/libtevent.so.0(_tevent_loop_once+0x8d)
[0x7f3a0697f31d]
     #22
/usr/lpp/mmfs/lib64/samba/libtevent.so.0(tevent_common_loop_wait+0x1b)
[0x7f3a0697f4bb]
     #23 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8107) [0x7f3a06983107]
     #24 /usr/lpp/mmfs/bin/smbd(main+0x1580) [0x7f3a07fb8fc0]
     #25 /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f3a03accb35]
     #26 /usr/lpp/mmfs/bin/smbd(+0x82c9) [0x7f3a07fb92c9]

I was able to recreate the panic on my test system with the attached
'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
smbtorture test. Please see also my proposed fix. Both patches apply on
master.

--
Regards

     Ralph Wuerthner


fix-delete-on-close-after-smb2_find.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
On Fri, Nov 03, 2017 at 04:10:23PM +0100, Ralph Wuerthner via samba-technical wrote:

> Hi!
>
> On a customer system I came recently across the following Samba panic:
>
> [2017/11/03 14:27:35.930242,  0, pid=446, effective(12000500,
> 12000513), real(12000500, 0)]
> ../source3/lib/util.c:791(smb_panic_s3)
>    PANIC (pid 446): assert failed: dirp->fsp->dptr->dir_hnd == dirp
> [2017/11/03 14:27:35.930722,  0, pid=446, effective(12000500,
> 12000513), real(12000500, 0)]
> ../source3/lib/util.c:902(log_stack_trace)
>    BACKTRACE: 27 stack frames:
>     #0 /usr/lpp/mmfs/lib64/libsmbconf.so.0(log_stack_trace+0x1a)
> [0x7f3a05013e7a]
>     #1 /usr/lpp/mmfs/lib64/libsmbconf.so.0(smb_panic_s3+0x20)
> [0x7f3a05013f50]
>     #2 /usr/lpp/mmfs/lib64/libsamba-util.so.0(smb_panic+0x2f)
> [0x7f3a07919bdf]
>     #3 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(+0xbb127)
> [0x7f3a07461127]
>     #4 /usr/lpp/mmfs/lib64/samba/libtalloc.so.2(_talloc_free+0x440)
> [0x7f3a06d8fed0]
>     #5 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(can_delete_directory_fsp+0x137)
> [0x7f3a07464357]
>     #6 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(can_set_delete_on_close+0x168)
> [0x7f3a074e85b8]
>     #7 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(+0xf4b73)
> [0x7f3a0749ab73]
>     #8 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_do_setfilepathinfo+0x169b)
> [0x7f3a074ab0ab]
>     #9 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_smb2_request_process_setinfo+0x630)
> [0x7f3a07504c30]
>     #10 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_smb2_request_dispatch+0xcb5)
> [0x7f3a074ecb05]
>     #11 /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(+0x148f22)
> [0x7f3a074eef22]
>     #12 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x9c9b) [0x7f3a06984c9b]
>     #13 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8167) [0x7f3a06983167]
>     #14
> /usr/lpp/mmfs/lib64/samba/libtevent.so.0(_tevent_loop_once+0x8d)
> [0x7f3a0697f31d]
>     #15 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(tevent_common_loop_wait+0x1b)
> [0x7f3a0697f4bb]
>     #16 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8107) [0x7f3a06983107]
>     #17
> /usr/lpp/mmfs/lib64/samba/libsmbd-base-samba4.so(smbd_process+0x721)
> [0x7f3a074db991]
>     #18 /usr/lpp/mmfs/bin/smbd(+0xb588) [0x7f3a07fbc588]
>     #19 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x9c9b) [0x7f3a06984c9b]
>     #20 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8167) [0x7f3a06983167]
>     #21
> /usr/lpp/mmfs/lib64/samba/libtevent.so.0(_tevent_loop_once+0x8d)
> [0x7f3a0697f31d]
>     #22 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(tevent_common_loop_wait+0x1b)
> [0x7f3a0697f4bb]
>     #23 /usr/lpp/mmfs/lib64/samba/libtevent.so.0(+0x8107) [0x7f3a06983107]
>     #24 /usr/lpp/mmfs/bin/smbd(main+0x1580) [0x7f3a07fb8fc0]
>     #25 /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f3a03accb35]
>     #26 /usr/lpp/mmfs/bin/smbd(+0x82c9) [0x7f3a07fb92c9]
>
> I was able to recreate the panic on my test system with the attached
> 'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
> smbtorture test. Please see also my proposed fix. Both patches apply
> on master.

Thanks *VERY* much for the test.

I'm trying to avoid adding calls that use by-pathname semantics,
so I'll take a look at the patch to see if we can fix it another
way first.

Thanks !

Jeremy.

>
>     Ralph Wuerthner
>

> From 634694af4c2c42206adaf49e8231f6555a1de4eb Mon Sep 17 00:00:00 2001
> From: Ralph Wuerthner <[hidden email]>
> Date: Wed, 1 Nov 2017 14:13:25 +0100
> Subject: [PATCH 1/2] s3: smbd: Fix delete-on-close after smb2_find
>
> Both dptr_create() and can_delete_directory_fsp() are calling OpenDir_fsp()
> to get a directory handle. This is causing an issue when delete-on-close is
> set after smb2_find because both directory handle instances share the same
> underlying file descriptor. In addition the SMB_ASSERT() in destructor
> smb_Dir_destructor() gets triggered.
> To avoid this use OpenDir() instead of OpenDir_fsp().
>
> Signed-off-by: Ralph Wuerthner <[hidden email]>
> ---
>  source3/smbd/dir.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 8d83fba..8e591ed 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -2132,9 +2132,9 @@ NTSTATUS can_delete_directory_fsp(files_struct *fsp)
>   char *talloced = NULL;
>   SMB_STRUCT_STAT st;
>   struct connection_struct *conn = fsp->conn;
> - struct smb_Dir *dir_hnd = OpenDir_fsp(talloc_tos(),
> + struct smb_Dir *dir_hnd = OpenDir(talloc_tos(),
>   conn,
> - fsp,
> + fsp->fsp_name,
>   NULL,
>   0);
>  
> --
> 2.7.4
>
>
> From 8399e0abd3cf8482fd7168ed017d03b9dff4e437 Mon Sep 17 00:00:00 2001
> From: Ralph Wuerthner <[hidden email]>
> Date: Fri, 27 Oct 2017 14:59:32 +0200
> Subject: [PATCH 2/2] Add FIND and set DOC test case.
>
> Signed-off-by: Ralph Wuerthner <[hidden email]>
> ---
>  source4/torture/smb2/delete-on-close.c | 67 ++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/source4/torture/smb2/delete-on-close.c b/source4/torture/smb2/delete-on-close.c
> index 44ef33e..b7c41e9 100644
> --- a/source4/torture/smb2/delete-on-close.c
> +++ b/source4/torture/smb2/delete-on-close.c
> @@ -516,6 +516,72 @@ static bool test_doc_create_if_exist(struct torture_context *tctx, struct smb2_t
>   return true;
>  }
>  
> +static bool test_doc_find_and_set_doc(struct torture_context *tctx, struct smb2_tree *tree)
> +{
> + struct smb2_create io;
> + struct smb2_find find;
> + NTSTATUS status;
> + union smb_search_data *d;
> + union smb_setfileinfo sfinfo;
> + unsigned int count;
> + uint32_t perms = 0;
> + int i;
> +
> + perms = SEC_STD_SYNCHRONIZE | SEC_STD_READ_CONTROL | SEC_STD_DELETE |
> + SEC_DIR_WRITE_ATTRIBUTE | SEC_DIR_READ_ATTRIBUTE |
> + SEC_DIR_WRITE_EA | SEC_FILE_APPEND_DATA |
> + SEC_FILE_WRITE_DATA | SEC_DIR_LIST;
> +
> + /* File should not exist for this first test, so make sure */
> + set_dir_delete_perms(tctx, tree);
> +
> + smb2_deltree(tree, DNAME);
> +
> + create_dir(tctx, tree);
> +
> + torture_comment(tctx, "FIND and delete directory\n");
> + torture_comment(tctx, "We expect NT_STATUS_OK\n");
> +
> + /* open the directory first */
> + ZERO_STRUCT(io);
> + io.in.desired_access = perms;
> + io.in.file_attributes = FILE_ATTRIBUTE_DIRECTORY;
> + io.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
> + io.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
> +   NTCREATEX_SHARE_ACCESS_DELETE;
> + io.in.create_options     = NTCREATEX_OPTIONS_DIRECTORY;
> + io.in.fname              = DNAME;
> +
> + status = smb2_create(tree, tctx, &io);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + /* list directory */
> + ZERO_STRUCT(find);
> + find.in.file.handle        = io.out.file.handle;
> + find.in.pattern            = "*";
> + find.in.continue_flags     = SMB2_CONTINUE_FLAG_SINGLE;
> + find.in.max_response_size  = 0x100;
> + find.in.level              = SMB2_FIND_BOTH_DIRECTORY_INFO;
> +
> + /* start enumeration on directory */
> + status = smb2_find_level(tree, tree, &find, &count, &d);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + /* set delete-on-close */
> + ZERO_STRUCT(sfinfo);
> + sfinfo.generic.level = RAW_SFILEINFO_DISPOSITION_INFORMATION;
> + sfinfo.disposition_info.in.delete_on_close = 1;
> + sfinfo.generic.in.file.handle = io.out.file.handle;
> + status = smb2_setinfo_file(tree, &sfinfo);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + /* close directory */
> + status = smb2_util_close(tree, io.out.file.handle);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + return true;
> +}
> +
> +
>  /*
>   *  Extreme testing of Delete On Close and permissions
>   */
> @@ -529,6 +595,7 @@ struct torture_suite *torture_smb2_doc_init(TALLOC_CTX *ctx)
>   torture_suite_add_1smb2_test(suite, "CREATE Existing", test_doc_create_exist);
>   torture_suite_add_1smb2_test(suite, "CREATE_IF", test_doc_create_if);
>   torture_suite_add_1smb2_test(suite, "CREATE_IF Existing", test_doc_create_if_exist);
> + torture_suite_add_1smb2_test(suite, "FIND and set DOC", test_doc_find_and_set_doc);
>  
>   suite->description = talloc_strdup(suite, "SMB2-Delete-on-Close-Perms tests");
>  
> --
> 2.7.4
>
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
On Fri, Nov 03, 2017 at 08:39:35AM -0700, Jeremy Allison via samba-technical wrote:

> > I was able to recreate the panic on my test system with the attached
> > 'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
> > smbtorture test. Please see also my proposed fix. Both patches apply
> > on master.
>
> Thanks *VERY* much for the test.
>
> I'm trying to avoid adding calls that use by-pathname semantics,
> so I'll take a look at the patch to see if we can fix it another
> way first.

Nope, we can't. Your patch is good :-). Not only that
it fixes an underlying issue I missed where calling
can_delete_directory_fsp() would certainly move the
directory pointer internally, thus causing a standard
directory listing to miss files.

I've logged bug:

https://bugzilla.samba.org/show_bug.cgi?id=13118

to track this.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
On Fri, Nov 03, 2017 at 11:59:02AM -0700, Jeremy Allison via samba-technical wrote:

> On Fri, Nov 03, 2017 at 08:39:35AM -0700, Jeremy Allison via samba-technical wrote:
> > > I was able to recreate the panic on my test system with the attached
> > > 'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
> > > smbtorture test. Please see also my proposed fix. Both patches apply
> > > on master.
> >
> > Thanks *VERY* much for the test.
> >
> > I'm trying to avoid adding calls that use by-pathname semantics,
> > so I'll take a look at the patch to see if we can fix it another
> > way first.
>
> Nope, we can't. Your patch is good :-). Not only that
> it fixes an underlying issue I missed where calling
> can_delete_directory_fsp() would certainly move the
> directory pointer internally, thus causing a standard
> directory listing to miss files.
>
> I've logged bug:
>
> https://bugzilla.samba.org/show_bug.cgi?id=13118
>
> to track this.
And here is your fix (slight reworking to remove
compile warning and commit message).

Can I get a second Team reviewer ?

Great work Ralph, thanks !

Jeremy.

master-13118.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Nov 03, 2017 at 11:59:02AM -0700, Jeremy Allison via samba-technical wrote:

> On Fri, Nov 03, 2017 at 08:39:35AM -0700, Jeremy Allison via samba-technical wrote:
> > > I was able to recreate the panic on my test system with the attached
> > > 'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
> > > smbtorture test. Please see also my proposed fix. Both patches apply
> > > on master.
> >
> > Thanks *VERY* much for the test.
> >
> > I'm trying to avoid adding calls that use by-pathname semantics,
> > so I'll take a look at the patch to see if we can fix it another
> > way first.
>
> Nope, we can't. Your patch is good :-). Not only that
> it fixes an underlying issue I missed where calling
> can_delete_directory_fsp() would certainly move the
> directory pointer internally, thus causing a standard
> directory listing to miss files.

I've tried too with a dup() on fsp->fh->fd, but that also moves the
offset. We need a new open() unfortunately.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
On Sun, Nov 05, 2017 at 08:24:32AM +0100, Volker Lendecke wrote:

> On Fri, Nov 03, 2017 at 11:59:02AM -0700, Jeremy Allison via samba-technical wrote:
> > On Fri, Nov 03, 2017 at 08:39:35AM -0700, Jeremy Allison via samba-technical wrote:
> > > > I was able to recreate the panic on my test system with the attached
> > > > 'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
> > > > smbtorture test. Please see also my proposed fix. Both patches apply
> > > > on master.
> > >
> > > Thanks *VERY* much for the test.
> > >
> > > I'm trying to avoid adding calls that use by-pathname semantics,
> > > so I'll take a look at the patch to see if we can fix it another
> > > way first.
> >
> > Nope, we can't. Your patch is good :-). Not only that
> > it fixes an underlying issue I missed where calling
> > can_delete_directory_fsp() would certainly move the
> > directory pointer internally, thus causing a standard
> > directory listing to miss files.
>
> I've tried too with a dup() on fsp->fh->fd, but that also moves the
> offset. We need a new open() unfortunately.

That's what this patch does. The change to an OpenDir() instead of
an OpenDir_fsp() means we're calling opendir() internally, not
fdopendir() so this patch does get a new fd.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: s3: smbd: Fix delete-on-close after smb2_find

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Sun, Nov 05, 2017 at 08:24:32AM +0100, Volker Lendecke wrote:

> On Fri, Nov 03, 2017 at 11:59:02AM -0700, Jeremy Allison via samba-technical wrote:
> > On Fri, Nov 03, 2017 at 08:39:35AM -0700, Jeremy Allison via samba-technical wrote:
> > > > I was able to recreate the panic on my test system with the attached
> > > > 'smb2.delete-on-close-perms.FIND and set DOC.FIND and set DOC'
> > > > smbtorture test. Please see also my proposed fix. Both patches apply
> > > > on master.
> > >
> > > Thanks *VERY* much for the test.
> > >
> > > I'm trying to avoid adding calls that use by-pathname semantics,
> > > so I'll take a look at the patch to see if we can fix it another
> > > way first.
> >
> > Nope, we can't. Your patch is good :-). Not only that
> > it fixes an underlying issue I missed where calling
> > can_delete_directory_fsp() would certainly move the
> > directory pointer internally, thus causing a standard
> > directory listing to miss files.
>
> I've tried too with a dup() on fsp->fh->fd, but that also moves the
> offset. We need a new open() unfortunately.

Oh, doh! You were agreeing with me, sorry. I read
your comment completely wrong.

Thanks for pushing the patch :-).