Fix concurrent changes on nscd aware files (BZ #23178)

As indicated by BZ#23178, concurrent access on some files read by nscd
may result non expected data send through service requisition.  This is
due 'sendfile' Linux implementation where for sockets with zero-copy
support, callers must ensure the transferred portions of the the file
reffered by input file descriptor remain unmodified until the reader
on the other end of socket has consumed the transferred data.

I could not find any explicit documentation stating this behaviour on
Linux kernel documentation.  However man-pages sendfile entry [1] states
in NOTES the aforementioned remark.  It was initially pushed on man-pages
with an explicit testcase [2] that shows changing the file used in
'sendfile' call prior the socket input data consumption results in
previous data being lost.

From commit message it stated on tested Linux version (3.15) only TCP
socket showed this issues, however on recent kernels (4.4) I noticed the
same behaviour for local sockets as well.

Since sendfile on HURD is a read/write operation and the underlying
issue on Linux, the straightforward fix is just remove sendfile use
altogether.  I am really skeptical it is hitting some hotstop (there
are indication over internet that sendfile is helpfull only for large
files, more than 10kb) here to justify that extra code complexity or
to pursuit other possible fix (through memory or file locks for
instance, which I am not sure it is doable).

Checked on x86_64-linux-gnu.

	[BZ #23178]
	* nscd/nscd-client.h (sendfileall): Remove prototype.
	* nscd/connections.c [HAVE_SENDFILE] (sendfileall): Remove function.
	(handle_request): Use writeall instead of sendfileall.
	* nscd/aicache.c (addhstaiX): Likewise.
	* nscd/grpcache.c (cache_addgr): Likewise.
	* nscd/hstcache.c (cache_addhst): Likewise.
	* nscd/initgrcache.c (addinitgroupsX): Likewise.
	* nscd/netgroupcache.c (addgetnetgrentX, addinnetgrX): Likewise.
	* nscd/pwdcache.c (cache_addpw): Likewise.
	* nscd/servicescache.c (cache_addserv): Likewise.
	* sysdeps/unix/sysv/linux/Makefile [$(subdir) == nscd]
	(sysdep-CFLAGS): Remove -DHAVE_SENDFILE.
	* sysdeps/unix/sysv/linux/kernel-features.h (__ASSUME_SENDFILE):
	Remove define.

[1] http://man7.org/linux/man-pages/man2/sendfile.2.html
[2] https://github.com/mkerrisk/man-pages/commit/7b6a3299776b5c1c4f169a591434a855d50c68b4#diff-efd6af3a70f0f07c578e85b51e83b3c3

diff --git a/ChangeLog b/ChangeLog
index 3d4f2373ffaaf342549401d29dd64033d35c62d0..adec735a6b08649786c2e413c0a4bb2aea99447a 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2018-05-16  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
+
+       [BZ #23178]
+       * nscd/nscd-client.h (sendfileall): Remove prototype.
+       * nscd/connections.c [HAVE_SENDFILE] (sendfileall): Remove function.
+       (handle_request): Use writeall instead of sendfileall.
+       * nscd/aicache.c (addhstaiX): Likewise.
+       * nscd/grpcache.c (cache_addgr): Likewise.
+       * nscd/hstcache.c (cache_addhst): Likewise.
+       * nscd/initgrcache.c (addinitgroupsX): Likewise.
+       * nscd/netgroupcache.c (addgetnetgrentX, addinnetgrX): Likewise.
+       * nscd/pwdcache.c (cache_addpw): Likewise.
+       * nscd/servicescache.c (cache_addserv): Likewise.
+       * sysdeps/unix/sysv/linux/Makefile [$(subdir) == nscd]
+       (sysdep-CFLAGS): Remove -DHAVE_SENDFILE.
+       * sysdeps/unix/sysv/linux/kernel-features.h (__ASSUME_SENDFILE):
+       Remove define.
+
 2018-05-16  H.J. Lu  <hongjiu.lu@intel.com>
 
        * sysdeps/x86_64/multiarch/strncat-c.c (STRNCAT_PRIMARY): Removed.

diff --git a/nscd/aicache.c b/nscd/aicache.c
index 6f7b038021746e22e39eb6f96688a27ef5c61b9a..2095edf911189b221f7f0c12f0a32dbdf559cf7d 100644 (file)
--- a/nscd/aicache.c
+++ b/nscd/aicache.c
@@ -31,9 +31,6 @@
 
 #include "dbg_log.h"
 #include "nscd.h"
-#ifdef HAVE_SENDFILE
-# include <kernel-features.h>
-#endif
 
 
 typedef enum nss_status (*nss_gethostbyname4_r)
@@ -447,32 +444,7 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
             would unnecessarily let the receiver wait.  */
          assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-         if (__builtin_expect (db->mmap_used, 1) && !alloca_used)
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) &dataset->resp > (char *) db->data);
-             assert ((char *) dataset - (char *) db->head + total
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-# ifndef __ASSUME_SENDFILE
-             ssize_t written;
-             written =
-# endif
-               sendfileall (fd, db->wr_fd, (char *) &dataset->resp
-                            - (char *) db->head, dataset->head.recsize);
-# ifndef __ASSUME_SENDFILE
-             if (written == -1 && errno == ENOSYS)
-               goto use_write;
-# endif
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           writeall (fd, &dataset->resp, dataset->head.recsize);
+         writeall (fd, &dataset->resp, dataset->head.recsize);
        }
 
       goto out;

diff --git a/nscd/connections.c b/nscd/connections.c
index 5f91985859b3026a1bda74e18dbaa60db0b8416a..1b3bae4eebdafa1bbc44d267f9652ff722962e40 100644 (file)
--- a/nscd/connections.c
+++ b/nscd/connections.c
@@ -46,9 +46,6 @@
 #include <sys/mman.h>
 #include <sys/param.h>
 #include <sys/poll.h>
-#ifdef HAVE_SENDFILE
-# include <sys/sendfile.h>
-#endif
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/un.h>
@@ -285,26 +282,6 @@ writeall (int fd, const void *buf, size_t len)
 }
 
 
-#ifdef HAVE_SENDFILE
-ssize_t
-sendfileall (int tofd, int fromfd, off_t off, size_t len)
-{
-  ssize_t n = len;
-  ssize_t ret;
-
-  do
-    {
-      ret = TEMP_FAILURE_RETRY (sendfile (tofd, fromfd, &off, n));
-      if (ret <= 0)
-       break;
-      n -= ret;
-    }
-  while (n > 0);
-  return ret < 0 ? ret : len - n;
-}
-#endif
-
-
 enum usekey
   {
     use_not = 0,
@@ -1163,35 +1140,8 @@ request from '%s' [%ld] not handled due to missing permission"),
       if (cached != NULL)
        {
          /* Hurray it's in the cache.  */
-         ssize_t nwritten;
-
-#ifdef HAVE_SENDFILE
-         if (__glibc_likely (db->mmap_used))
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) cached->data > (char *) db->data);
-             assert ((char *) cached->data - (char *) db->head
-                     + cached->recsize
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-             nwritten = sendfileall (fd, db->wr_fd,
-                                     (char *) cached->data
-                                     - (char *) db->head, cached->recsize);
-# ifndef __ASSUME_SENDFILE
-             if (nwritten == -1 && errno == ENOSYS)
-               goto use_write;
-# endif
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           nwritten = writeall (fd, cached->data, cached->recsize);
-
-         if (nwritten != cached->recsize
-             && __builtin_expect (debug_level, 0) > 0)
+         if (writeall (fd, cached->data, cached->recsize) != cached->recsize
+             && __glibc_unlikely (debug_level > 0))
            {
              /* We have problems sending the result.  */
              char buf[256];

diff --git a/nscd/grpcache.c b/nscd/grpcache.c
index 0ed8e656b2ae8ff9dc6e5f37c7f04c7ab9507741..c01aeb1f1ddda1b34ad1de5e4840a8f8055a75e0 100644 (file)
--- a/nscd/grpcache.c
+++ b/nscd/grpcache.c
@@ -35,9 +35,6 @@
 
 #include "nscd.h"
 #include "dbg_log.h"
-#ifdef HAVE_SENDFILE
-# include <kernel-features.h>
-#endif
 
 /* This is the standard reply in case the service is disabled.  */
 static const gr_response_header disabled =
@@ -318,37 +315,9 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
             unnecessarily let the receiver wait.  */
          assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-         if (__builtin_expect (db->mmap_used, 1) && ! dataset_temporary)
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) &dataset->resp > (char *) db->data);
-             assert ((char *) dataset - (char *) db->head
-                     + total
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-             ssize_t written = sendfileall (fd, db->wr_fd,
-                                            (char *) &dataset->resp
-                                            - (char *) db->head,
-                                            dataset->head.recsize);
-             if (written != dataset->head.recsize)
-               {
-# ifndef __ASSUME_SENDFILE
-                 if (written == -1 && errno == ENOSYS)
-                   goto use_write;
-# endif
-                 all_written = false;
-               }
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           if (writeall (fd, &dataset->resp, dataset->head.recsize)
-               != dataset->head.recsize)
-             all_written = false;
+         if (writeall (fd, &dataset->resp, dataset->head.recsize)
+             != dataset->head.recsize)
+           all_written = false;
        }
 
       /* Add the record to the database.  But only if it has not been

diff --git a/nscd/hstcache.c b/nscd/hstcache.c
index 344a2b3052cc6ee0fb05f09b4f51a5cb7a147848..6ef0c653eae54b6c7fb9db692caf1d9179364773 100644 (file)
--- a/nscd/hstcache.c
+++ b/nscd/hstcache.c
@@ -37,9 +37,6 @@
 
 #include "nscd.h"
 #include "dbg_log.h"
-#ifdef HAVE_SENDFILE
-# include <kernel-features.h>
-#endif
 
 
 /* This is the standard reply in case the service is disabled.  */
@@ -352,37 +349,9 @@ cache_addhst (struct database_dyn *db, int fd, request_header *req,
             unnecessarily keep the receiver waiting.  */
          assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-         if (__builtin_expect (db->mmap_used, 1) && !alloca_used)
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) &dataset->resp > (char *) db->data);
-             assert ((char *) dataset - (char *) db->head
-                     + total
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-             ssize_t written = sendfileall (fd, db->wr_fd,
-                                            (char *) &dataset->resp
-                                            - (char *) db->head,
-                                            dataset->head.recsize);
-             if (written != dataset->head.recsize)
-               {
-# ifndef __ASSUME_SENDFILE
-                 if (written == -1 && errno == ENOSYS)
-                   goto use_write;
-# endif
-                 all_written = false;
-               }
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           if (writeall (fd, &dataset->resp, dataset->head.recsize)
-               != dataset->head.recsize)
-             all_written = false;
+         if (writeall (fd, &dataset->resp, dataset->head.recsize)
+             != dataset->head.recsize)
+           all_written = false;
        }
 
       /* Add the record to the database.  But only if it has not been

diff --git a/nscd/initgrcache.c b/nscd/initgrcache.c
index e21068fbf0f3f2f2e4a917341e0b172989dcab99..2c74951f579f4afd2bef2609685d0a765b5fcc76 100644 (file)
--- a/nscd/initgrcache.c
+++ b/nscd/initgrcache.c
@@ -29,9 +29,6 @@
 
 #include "dbg_log.h"
 #include "nscd.h"
-#ifdef HAVE_SENDFILE
-# include <kernel-features.h>
-#endif
 
 #include "../nss/nsswitch.h"
 
@@ -353,37 +350,9 @@ addinitgroupsX (struct database_dyn *db, int fd, request_header *req,
             unnecessarily let the receiver wait.  */
          assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-         if (__builtin_expect (db->mmap_used, 1) && !alloca_used)
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) &dataset->resp > (char *) db->data);
-             assert ((char *) dataset - (char *) db->head
-                     + total
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-             ssize_t written = sendfileall (fd, db->wr_fd,
-                                            (char *) &dataset->resp
-                                            - (char *) db->head,
-                                            dataset->head.recsize);
-             if (written != dataset->head.recsize)
-               {
-# ifndef __ASSUME_SENDFILE
-                 if (written == -1 && errno == ENOSYS)
-                   goto use_write;
-# endif
-                 all_written = false;
-               }
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           if (writeall (fd, &dataset->resp, dataset->head.recsize)
-               != dataset->head.recsize)
-             all_written = false;
+         if (writeall (fd, &dataset->resp, dataset->head.recsize)
+             != dataset->head.recsize)
+           all_written = false;
        }
 
 

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 2f187b208c19cec983f401c03a32344733de254a..2b35389cc816c3c88384783664e937766fef0b79 100644 (file)
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -413,33 +413,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
         since while inserting this thread might block and so would
         unnecessarily let the receiver wait.  */
     writeout:
-#ifdef HAVE_SENDFILE
-      if (__builtin_expect (db->mmap_used, 1) && cacheable)
-       {
-         assert (db->wr_fd != -1);
-         assert ((char *) &dataset->resp > (char *) db->data);
-         assert ((char *) dataset - (char *) db->head + total
-                 <= (sizeof (struct database_pers_head)
-                     + db->head->module * sizeof (ref_t)
-                     + db->head->data_size));
-# ifndef __ASSUME_SENDFILE
-         ssize_t written =
-# endif
-           sendfileall (fd, db->wr_fd, (char *) &dataset->resp
-                        - (char *) db->head, dataset->head.recsize);
-# ifndef __ASSUME_SENDFILE
-         if (written == -1 && errno == ENOSYS)
-           goto use_write;
-# endif
-       }
-      else
-#endif
-       {
-#if defined HAVE_SENDFILE && !defined __ASSUME_SENDFILE
-       use_write:
-#endif
-         writeall (fd, &dataset->resp, dataset->head.recsize);
-       }
+      writeall (fd, &dataset->resp, dataset->head.recsize);
     }
 
   if (cacheable)
@@ -594,36 +568,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
       /* We write the dataset before inserting it to the database
         since while inserting this thread might block and so would
         unnecessarily let the receiver wait.  */
-       assert (fd != -1);
+      assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-      if (__builtin_expect (db->mmap_used, 1) && cacheable)
-       {
-         assert (db->wr_fd != -1);
-         assert ((char *) &dataset->resp > (char *) db->data);
-         assert ((char *) dataset - (char *) db->head + sizeof (*dataset)
-                 <= (sizeof (struct database_pers_head)
-                     + db->head->module * sizeof (ref_t)
-                     + db->head->data_size));
-# ifndef __ASSUME_SENDFILE
-         ssize_t written =
-# endif
-           sendfileall (fd, db->wr_fd,
-                        (char *) &dataset->resp - (char *) db->head,
-                        sizeof (innetgroup_response_header));
-# ifndef __ASSUME_SENDFILE
-         if (written == -1 && errno == ENOSYS)
-           goto use_write;
-# endif
-       }
-      else
-#endif
-       {
-#if defined HAVE_SENDFILE && !defined __ASSUME_SENDFILE
-       use_write:
-#endif
-         writeall (fd, &dataset->resp, sizeof (innetgroup_response_header));
-       }
+      writeall (fd, &dataset->resp, sizeof (innetgroup_response_header));
     }
 
   if (cacheable)

diff --git a/nscd/nscd-client.h b/nscd/nscd-client.h
index 831eb5acbf2c9b14c01f7b84f7019ef7d6b965f8..624effabb4bc273812bc3c8b4d4819c98a0b5580 100644 (file)
--- a/nscd/nscd-client.h
+++ b/nscd/nscd-client.h
@@ -446,8 +446,6 @@ extern ssize_t __readvall (int fd, const struct iovec *iov, int iovcnt)
   attribute_hidden;
 extern ssize_t writeall (int fd, const void *buf, size_t len)
   attribute_hidden;
-extern ssize_t sendfileall (int tofd, int fromfd, off_t off, size_t len)
-  attribute_hidden;
 
 /* Get netlink timestamp counter from mapped area or zero.  */
 extern uint32_t __nscd_get_nl_timestamp (void)

diff --git a/nscd/pwdcache.c b/nscd/pwdcache.c
index 4c3ab660211f2b289143bdee94c9a117ae4d504e..997d7c08fc54f6b16bd17b975e0601a203a0b451 100644 (file)
--- a/nscd/pwdcache.c
+++ b/nscd/pwdcache.c
@@ -35,9 +35,6 @@
 
 #include "nscd.h"
 #include "dbg_log.h"
-#ifdef HAVE_SENDFILE
-# include <kernel-features.h>
-#endif
 
 /* This is the standard reply in case the service is disabled.  */
 static const pw_response_header disabled =
@@ -296,37 +293,9 @@ cache_addpw (struct database_dyn *db, int fd, request_header *req,
             unnecessarily let the receiver wait.  */
          assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-         if (__builtin_expect (db->mmap_used, 1) && !alloca_used)
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) &dataset->resp > (char *) db->data);
-             assert ((char *) dataset - (char *) db->head
-                     + total
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-             ssize_t written = sendfileall (fd, db->wr_fd,
-                                            (char *) &dataset->resp
-                                            - (char *) db->head,
-                                            dataset->head.recsize);
-             if (written != dataset->head.recsize)
-               {
-# ifndef __ASSUME_SENDFILE
-                 if (written == -1 && errno == ENOSYS)
-                   goto use_write;
-# endif
-                 all_written = false;
-               }
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           if (writeall (fd, &dataset->resp, dataset->head.recsize)
-               != dataset->head.recsize)
-             all_written = false;
+         if (writeall (fd, &dataset->resp, dataset->head.recsize)
+             != dataset->head.recsize)
+           all_written = false;
        }
 
 

diff --git a/nscd/servicescache.c b/nscd/servicescache.c
index 49d9d0dece186c1bdd5fce3769db24c29e43d396..187207f7eae26342b4290a75cb5153b68621c360 100644 (file)
--- a/nscd/servicescache.c
+++ b/nscd/servicescache.c
@@ -278,37 +278,9 @@ cache_addserv (struct database_dyn *db, int fd, request_header *req,
             unnecessarily keep the receiver waiting.  */
          assert (fd != -1);
 
-#ifdef HAVE_SENDFILE
-         if (__builtin_expect (db->mmap_used, 1) && !alloca_used)
-           {
-             assert (db->wr_fd != -1);
-             assert ((char *) &dataset->resp > (char *) db->data);
-             assert ((char *) dataset - (char *) db->head
-                     + total
-                     <= (sizeof (struct database_pers_head)
-                         + db->head->module * sizeof (ref_t)
-                         + db->head->data_size));
-             ssize_t written = sendfileall (fd, db->wr_fd,
-                                            (char *) &dataset->resp
-                                            - (char *) db->head,
-                                            dataset->head.recsize);
-             if (written != dataset->head.recsize)
-               {
-# ifndef __ASSUME_SENDFILE
-                 if (written == -1 && errno == ENOSYS)
-                   goto use_write;
-# endif
-                 all_written = false;
-               }
-           }
-         else
-# ifndef __ASSUME_SENDFILE
-         use_write:
-# endif
-#endif
-           if (writeall (fd, &dataset->resp, dataset->head.recsize)
-               != dataset->head.recsize)
-             all_written = false;
+         if (writeall (fd, &dataset->resp, dataset->head.recsize)
+             != dataset->head.recsize)
+           all_written = false;
        }
 
       /* Add the record to the database.  But only if it has not been

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 8f19e0efc35ec080b86ac85f2886f8d09a15021a..cd6244f7ea2471b4a86286b03f1cd0543a900a06 100644 (file)
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -190,7 +190,7 @@ CFLAGS-mq_receive.c += -fexceptions
 endif
 
 ifeq ($(subdir),nscd)
-sysdep-CFLAGS += -DHAVE_EPOLL -DHAVE_SENDFILE -DHAVE_INOTIFY -DHAVE_NETLINK
+sysdep-CFLAGS += -DHAVE_EPOLL -DHAVE_INOTIFY -DHAVE_NETLINK
 CFLAGS-gai.c += -DNEED_NETLINK
 endif
 

diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index f65a262994befa04368bdfd37dac87ee109ca84a..2ef2916a52418a43e79121407027e151698645e4 100644 (file)
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -37,9 +37,6 @@
    introduced.  If somebody cares these values can afterwards be
    corrected.  */
 
-/* The sendfile syscall was introduced in 2.2.0.  */
-#define __ASSUME_SENDFILE              1
-
 /* Some architectures use the socketcall multiplexer for some or all
    socket-related operations instead of separate syscalls.
    __ASSUME_SOCKETCALL is defined for such architectures.  */