[PATCH] avoid stat of foreign file systems in quota code
Jeremy Allison
jra at samba.org
Tue Jan 26 21:37:08 UTC 2016
On Tue, Jan 19, 2016 at 12:07:22AM +0200, Uri Simchoni wrote:
> Hi,
>
> Following the discussion on vfs_fileid, I realized that we may have
> the same issue in the quota code. Whenever you type "dir" at a
> Windows command prompt on a samba share, the client asks for free
> space and activates the quota code.
>
> Attached is a fix that avoids stating mounted file systems that
> cannot be "ours".
>
> - This patch is independent of the refactoring patch series that I
> just re-sent
> - I haven't actually gone through the effort of verifying that there
> is an issue, reproducing it, and seeing that the patch fixes it.
> Just verified no degradation on my system. So I'm not sure whether
> this patch falls under "bugfix" (that should be backported if it's
> good) or "eliminating not-so-nice behavior".
That's a really nice patch - thanks ! Clever optimization.
Pushed.
> From 59e1f102720e4670018c7bff72b09095a0940121 Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Mon, 18 Jan 2016 23:34:06 +0200
> Subject: [PATCH 1/2] s3-lib: introduce sys_realpath()
>
> Add sys_realpath() function that captures the OS variations
> on realpath().
>
> Signed-off-by: Uri Simchoni <uri at samba.org>
> ---
> source3/include/proto.h | 1 +
> source3/lib/system.c | 25 +++++++++++++++++++++++++
> source3/modules/vfs_default.c | 15 +--------------
> 3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index 809cb95..cd54ee6 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -264,6 +264,7 @@ int sys_setxattr (const char *path, const char *name, const void *value, size_t
> int sys_fsetxattr (int filedes, const char *name, const void *value, size_t size, int flags);
> uint32_t unix_dev_major(SMB_DEV_T dev);
> uint32_t unix_dev_minor(SMB_DEV_T dev);
> +char *sys_realpath(const char *path);
> #if 0
> int sys_get_number_of_cores(void);
> #endif
> diff --git a/source3/lib/system.c b/source3/lib/system.c
> index e54b946..0351e37 100644
> --- a/source3/lib/system.c
> +++ b/source3/lib/system.c
> @@ -1236,6 +1236,31 @@ uint32_t unix_dev_minor(SMB_DEV_T dev)
> #endif
> }
>
> +/**************************************************************************
> + Wrapper for realpath.
> +****************************************************************************/
> +
> +char *sys_realpath(const char *path)
> +{
> + char *result;
> +
> +#ifdef REALPATH_TAKES_NULL
> + result = realpath(path, NULL);
> +#else
> + result = SMB_MALLOC_ARRAY(char, PATH_MAX + 1);
> + if (result) {
> + char *resolved_path = realpath(path, result);
> + if (!resolved_path) {
> + SAFE_FREE(result);
> + } else {
> + /* SMB_ASSERT(result == resolved_path) ? */
> + result = resolved_path;
> + }
> + }
> +#endif
> + return result;
> +}
> +
> #if 0
> /*******************************************************************
> Return the number of CPUs.
> diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
> index 819a1a1..f8a70ba 100644
> --- a/source3/modules/vfs_default.c
> +++ b/source3/modules/vfs_default.c
> @@ -2141,20 +2141,7 @@ static char *vfswrap_realpath(vfs_handle_struct *handle, const char *path)
> char *result;
>
> START_PROFILE(syscall_realpath);
> -#ifdef REALPATH_TAKES_NULL
> - result = realpath(path, NULL);
> -#else
> - result = SMB_MALLOC_ARRAY(char, PATH_MAX+1);
> - if (result) {
> - char *resolved_path = realpath(path, result);
> - if (!resolved_path) {
> - SAFE_FREE(result);
> - } else {
> - /* SMB_ASSERT(result == resolved_path) ? */
> - result = resolved_path;
> - }
> - }
> -#endif
> + result = sys_realpath(path);
> END_PROFILE(syscall_realpath);
> return result;
> }
> --
> 2.4.3
>
>
> From 22d6732e70aef27fa69474ef35a75e07461e5a92 Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Mon, 18 Jan 2016 23:34:48 +0200
> Subject: [PATCH 2/2] smbd quotas: avoid stat of foreign file systems
>
> When determining the block device of our file system, avoid
> stat'ing paths which are definitely not the mount point of
> our file system. This is done to avoid stalling smbd due to
> unresponsive network file systems (e.g. NFS) which are not
> related to the SMB shares.
>
> See discussion in samba-technical for vfs_fileid:
> https://lists.samba.org/archive/samba-technical/2016-January/111553.html
>
> Signed-off-by: Uri Simchoni <uri at samba.org>
> ---
> source3/lib/sysquotas.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/source3/lib/sysquotas.c b/source3/lib/sysquotas.c
> index 2ef1d1b..bacc4b2 100644
> --- a/source3/lib/sysquotas.c
> +++ b/source3/lib/sysquotas.c
> @@ -40,35 +40,84 @@
>
> #endif /* NO_QUOTACTL_USED */
>
> -#ifdef HAVE_MNTENT
> +#if defined(HAVE_MNTENT) && defined(HAVE_REALPATH)
> static int sys_path_to_bdev(const char *path, char **mntpath, char **bdev, char **fs)
> {
> int ret = -1;
> SMB_STRUCT_STAT S;
> FILE *fp;
> - struct mntent *mnt;
> + struct mntent *mnt = NULL;
> SMB_DEV_T devno;
> + char *stat_mntpath = NULL;
> + char *p;
>
> /* find the block device file */
> -
> - if (!path||!mntpath||!bdev||!fs)
> - smb_panic("sys_path_to_bdev: called with NULL pointer");
> -
> (*mntpath) = NULL;
> (*bdev) = NULL;
> (*fs) = NULL;
> -
> - if ( sys_stat(path, &S, false) == -1 )
> - return (-1);
> +
> + if (sys_stat(path, &S, false) != 0) {
> + return -1;
> + }
>
> devno = S.st_ex_dev ;
>
> + stat_mntpath = sys_realpath(path);
> + if (stat_mntpath == NULL) {
> + DBG_WARNING("realpath(%s) failed - %s\n", path,
> + strerror(errno));
> + goto out;
> + }
> +
> + if (sys_stat(stat_mntpath, &S, false) != 0) {
> + DBG_WARNING("cannot stat real path %s - %s\n", stat_mntpath,
> + strerror(errno));
> + goto out;
> + }
> +
> + if (S.st_ex_dev != devno) {
> + DBG_WARNING("device on real path has changed\n");
> + goto out;
> + }
> +
> + while (true) {
> + p = strrchr(stat_mntpath, '/');
> + if (p == NULL) {
> + DBG_ERR("realpath for %s does not begin with a '/'\n",
> + path);
> + goto out;
> + }
> +
> + if (p == stat_mntpath) {
> + ++p;
> + }
> +
> + *p = 0;
> + if (sys_stat(stat_mntpath, &S, false) != 0) {
> + DBG_WARNING("cannot stat real path component %s - %s\n",
> + stat_mntpath, strerror(errno));
> + goto out;
> + }
> + if (S.st_ex_dev != devno) {
> + *p = '/';
> + break;
> + }
> +
> + if (p <= stat_mntpath + 1) {
> + break;
> + }
> + }
> +
> fp = setmntent(MOUNTED,"r");
> if (fp == NULL) {
> - return -1;
> + goto out;
> }
>
> while ((mnt = getmntent(fp))) {
> + if (!strequal(mnt->mnt_dir, stat_mntpath)) {
> + continue;
> + }
> +
> if ( sys_stat(mnt->mnt_dir, &S, false) == -1 )
> continue ;
>
> @@ -91,6 +140,8 @@ static int sys_path_to_bdev(const char *path, char **mntpath, char **bdev, char
>
> endmntent(fp) ;
>
> +out:
> + SAFE_FREE(stat_mntpath);
> return ret;
> }
> /* #endif HAVE_MNTENT */
> --
> 2.4.3
>
More information about the samba-technical
mailing list