[PATCH] Bunch of options for vfs_fileid

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

[PATCH] Bunch of options for vfs_fileid

Samba - samba-technical mailing list
Hi!

Attached is a patchset with a bunch of new options for vfs_fileid. They have
been in use downstream by a vendor for quite some time and I would like to bring
them upstream.

The following options are added:

  fileid:fstype deny = LIST
  fileid:fstype allow = LIST
  fileid:mntdir deny = LIST
  fileid:mntdir allow = LIST

  fileid:algorithm = hostname
  fileid:algorithm = fsname_nodirs
  fileid:nolockinode
  fileid:algorithm = fsname_norootdir

The patch that adds the fstype/mntdir options has been discussed previously
about two years ago:
https://lists.samba.org/archive/samba-technical/2016-January/111553.html

Afaict consensus was that it's a useful addition and concerns raised by Jeremey
where addressed by better explanation of the use case. The patch then never went
upstream though.

All parameters are explained in manpage updates.

The use case of the new algorithms is to mitigate contention on locking.tdb
records in a cluster.

"fileid:algorithm = fsname_norootdir" is probably the most useful one as it
mitigates contention on the locking.tdb record on the root directory of shares
which was seen frequently with certain workloads.

I'm wrapping all patches in one patchset, as the patches build on top of one
another, eg the algorithm added for "fileid:algorithm = hostname" is used by the
the other new algorithm.

Please review&push if happy. Thanks!

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

vfs_fileid.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Bunch of options for vfs_fileid

Samba - samba-technical mailing list
On Fri, Jan 05, 2018 at 10:44:27AM +0100, Ralph Böhme wrote:

> Hi!
>
> Attached is a patchset with a bunch of new options for vfs_fileid. They have
> been in use downstream by a vendor for quite some time and I would like to bring
> them upstream.
>
> The following options are added:
>
>   fileid:fstype deny = LIST
>   fileid:fstype allow = LIST
>   fileid:mntdir deny = LIST
>   fileid:mntdir allow = LIST
>
>   fileid:algorithm = hostname
>   fileid:algorithm = fsname_nodirs
>   fileid:nolockinode
>   fileid:algorithm = fsname_norootdir
>
> The patch that adds the fstype/mntdir options has been discussed previously
> about two years ago:
> https://lists.samba.org/archive/samba-technical/2016-January/111553.html
>
> Afaict consensus was that it's a useful addition and concerns raised by Jeremey
> where addressed by better explanation of the use case. The patch then never went
> upstream though.

Thanks for the link on the discussion - that helped refresh my memory.

> All parameters are explained in manpage updates.
>
> The use case of the new algorithms is to mitigate contention on locking.tdb
> records in a cluster.
>
> "fileid:algorithm = fsname_norootdir" is probably the most useful one as it
> mitigates contention on the locking.tdb record on the root directory of shares
> which was seen frequently with certain workloads.
>
> I'm wrapping all patches in one patchset, as the patches build on top of one
> another, eg the algorithm added for "fileid:algorithm = hostname" is used by the
> the other new algorithm.
>
> Please review&push if happy. Thanks!

LGTM, RB+ and pushed - thanks !

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Bunch of options for vfs_fileid

Samba - samba-technical mailing list
Hi Jeremy,

>> Please review&push if happy. Thanks!
>
> LGTM, RB+ and pushed - thanks !

I am not sure if the following two spots are correct.
Maybe you can give them a second look?

> + char hostname[HOST_NAME_MAX+1];
[...]
> +
> + rc = gethostname(hostname, HOST_NAME_MAX+1);
> + if (rc != 0) {
> + DBG_ERR("gethostname failed\n");
> + return UINT64_MAX;
> + }
man gethostname on Linux notes a discrepancy between Linux and POSIX:
in POSIX gethostname is allowed to return up to 255 characters, while
HOST_NAME_MAX is just 64 characters on Linux. Not sure how this looks
like on other platforms. Maybe it makes more sense to use an array of
size 256 to be on the safe side?
The error path that returns the same number for all devices might be
problematic too, as it will then generate the same fileid for files
with the same inode number on different filesystems.

> + TALLOC_FREE(devname);
> +
> + id = fileid_uint64_hash((uint8_t *)devname, devname_len);

Isn't this a use-after-free?

Regards,
Christian



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Bunch of options for vfs_fileid

Samba - samba-technical mailing list
Hi Christian,

On Sat, Jan 06, 2018 at 08:53:01AM +0100, Christian Ambach wrote:

> > + char hostname[HOST_NAME_MAX+1];
> [...]
> > +
> > + rc = gethostname(hostname, HOST_NAME_MAX+1);
> > + if (rc != 0) {
> > + DBG_ERR("gethostname failed\n");
> > + return UINT64_MAX;
> > + }
> man gethostname on Linux notes a discrepancy between Linux and POSIX:
> in POSIX gethostname is allowed to return up to 255 characters, while
> HOST_NAME_MAX is just 64 characters on Linux. Not sure how this looks
> like on other platforms. Maybe it makes more sense to use an array of
> size 256 to be on the safe side?
Why? As long as the array size matches the size passed to gethostname()
everything's fine.

> The error path that returns the same number for all devices might be
> problematic too, as it will then generate the same fileid for files
> with the same inode number on different filesystems.

Yeah, that's a bit problematic in theory. In practice, afaict gethostname() will
only fail if the passed in buffer size is too small, but this can't happen when
using HOST_NAME_MAX.

> > + TALLOC_FREE(devname);
> > +
> > + id = fileid_uint64_hash((uint8_t *)devname, devname_len);
>
> Isn't this a use-after-free?

You bet it is. Damn! Thanks for catching this. Patch attached.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

uaf.patch (881 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Bunch of options for vfs_fileid

Samba - samba-technical mailing list
On Sat, Jan 06, 2018 at 04:33:03PM +0100, Ralph Böhme via samba-technical wrote:

> Hi Christian,
>
> On Sat, Jan 06, 2018 at 08:53:01AM +0100, Christian Ambach wrote:
> > > + char hostname[HOST_NAME_MAX+1];
> > [...]
> > > +
> > > + rc = gethostname(hostname, HOST_NAME_MAX+1);
> > > + if (rc != 0) {
> > > + DBG_ERR("gethostname failed\n");
> > > + return UINT64_MAX;
> > > + }
> > man gethostname on Linux notes a discrepancy between Linux and POSIX:
> > in POSIX gethostname is allowed to return up to 255 characters, while
> > HOST_NAME_MAX is just 64 characters on Linux. Not sure how this looks
> > like on other platforms. Maybe it makes more sense to use an array of
> > size 256 to be on the safe side?
>
> Why? As long as the array size matches the size passed to gethostname()
> everything's fine.
>
> > The error path that returns the same number for all devices might be
> > problematic too, as it will then generate the same fileid for files
> > with the same inode number on different filesystems.
>
> Yeah, that's a bit problematic in theory. In practice, afaict gethostname() will
> only fail if the passed in buffer size is too small, but this can't happen when
> using HOST_NAME_MAX.
>
> > > + TALLOC_FREE(devname);
> > > +
> > > + id = fileid_uint64_hash((uint8_t *)devname, devname_len);
> >
> > Isn't this a use-after-free?
>
> You bet it is. Damn! Thanks for catching this. Patch attached.

Ralph I'm sorry, I missed that ! I'll add this on top.

> --
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From e05f315cb4fd93ec2924a0d528f90bc601445a8e Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Sat, 6 Jan 2018 16:13:52 +0100
> Subject: [PATCH] vfs_fileid: fix a use after free
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/modules/vfs_fileid.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/source3/modules/vfs_fileid.c b/source3/modules/vfs_fileid.c
> index 98cc32d62d5..c890876c998 100644
> --- a/source3/modules/vfs_fileid.c
> +++ b/source3/modules/vfs_fileid.c
> @@ -233,9 +233,11 @@ static uint64_t fileid_device_mapping_hostname(struct fileid_handle_data *data,
>   return UINT64_MAX;
>   }
>   devname_len = talloc_array_length(devname) - 1;
> - TALLOC_FREE(devname);
>  
>   id = fileid_uint64_hash((uint8_t *)devname, devname_len);
> +
> + TALLOC_FREE(devname);
> +
>   return id;
>  }
>  
> --
> 2.13.6
>