Fix tcp outgoing tos bugs

The tcp_outgoing_tos is buggy in trunk:
 - The ToS is never set for packets of the first request of a TCP connection.
 - The ToS is never set for HTTPS traffic no matter whether requests are bumped
   or not.
 - The ToS value is not set for ftp data connections

This patch solve the above problems:
 - It moves the codes which sets the TOS value for a new connection from the
   the comm_openex to a higher-level code, where the connection protocol
   (IPv4 or IPv6) is known.
 - Add code to set TOS value for ftp data connections.
 - Add a check on parsing code to warn users if the configured ToS value has the
   ECN bits set, and adjust the value to a correct one.

Notes
  Currently squid support only passive ftp data connections. If squid in the
future supports active ftp connections, then some work required to TcpAcceptor
class to allow setting ToS values for connections established on squid listening
sockets.

This is a Measurement Factory project

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 9936d79bb8cb2cff653b35cdb5ceb3cb0b57fe17..bbedfe63bcb9a3936273a669d0f632f183d757af 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -825,6 +825,20 @@ FwdState::connectStart()
             if (pinned_connection->pinnedAuth())
                 request->flags.auth = true;
             comm_add_close_handler(serverConn->fd, fwdServerClosedWrapper, this);
+
+            /* Update server side TOS and Netfilter mark on the connection. */
+            if (Ip::Qos::TheConfig.isAclTosActive()) {
+                debugs(17, 3, HERE << "setting tos for pinned connection to " << (int)serverConn->tos );
+                serverConn->tos = GetTosToServer(request);
+                Ip::Qos::setSockTos(serverConn, serverConn->tos);
+            }
+#if SO_MARK
+            if (Ip::Qos::TheConfig.isAclNfmarkActive()) {
+                serverConn->nfmark = GetNfmarkToServer(request);
+                Ip::Qos::setSockNfmark(serverConn, serverConn->nfmark);
+            }
+#endif
+
             // the server may close the pinned connection before this request
             pconnRace = racePossible;
             dispatch();

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 413379339f1dde0a69564755bb16b1b75cb6417f..aefc3bbe15c9913d71d657a8aa4da7bbb19259cc 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -1463,6 +1463,12 @@ parse_acl_tos(acl_tos ** head)
         return;
     }
 
+    const unsigned int chTos = tos & 0xFC;
+    if (chTos != tos) {
+        debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Tos value '" << tos << "' adjusted to '" << chTos << "'");
+        tos = chTos;
+    }
+
     CBDATA_INIT_TYPE_FREECB(acl_tos, freed_acl_tos);
 
     l = cbdataAlloc(acl_tos);

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 11603383ef576ab8fb41737ce8ebc2bb2f00b0b1..3915bbec47d7a5b11cbbaa462a8faad9e55ea764 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -1893,6 +1893,8 @@ DOC_START
 
        Processing proceeds in the order specified, and stops at first fully
        matching line.
+
+       Only fast ACLs are supported.
 DOC_END
 
 NAME: clientside_tos
@@ -1935,6 +1937,8 @@ DOC_START
        acl good_service_net src 10.0.1.0/24
        tcp_outgoing_mark 0x00 normal_service_net
        tcp_outgoing_mark 0x20 good_service_net
+
+       Only fast ACLs are supported.
 DOC_END
 
 NAME: clientside_mark

diff --git a/src/comm.cc b/src/comm.cc
index 34b0ceb1714107f954207e7b9c526b5b111b60b2..735eab3236c0ca8d42d07330312d510ed52a8c34 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -82,7 +82,7 @@
  */
 
 static IOCB commHalfClosedReader;
-static void comm_init_opened(const Comm::ConnectionPointer &conn, tos_t tos, nfmark_t nfmark, const char *note, struct addrinfo *AI);
+static void comm_init_opened(const Comm::ConnectionPointer &conn, const char *note, struct addrinfo *AI);
 static int comm_apply_flags(int new_socket, Ip::Address &addr, int flags, struct addrinfo *AI);
 
 #if USE_DELAY_POOLS
@@ -245,7 +245,7 @@ comm_open(int sock_type,
           int flags,
           const char *note)
 {
-    return comm_openex(sock_type, proto, addr, flags, 0, 0, note);
+    return comm_openex(sock_type, proto, addr, flags, note);
 }
 
 void
@@ -258,7 +258,7 @@ comm_open_listener(int sock_type,
     conn->flags |= COMM_DOBIND;
 
     /* attempt native enabled port. */
-    conn->fd = comm_openex(sock_type, proto, conn->local, conn->flags, 0, 0, note);
+    conn->fd = comm_openex(sock_type, proto, conn->local, conn->flags, note);
 }
 
 int
@@ -274,7 +274,7 @@ comm_open_listener(int sock_type,
     flags |= COMM_DOBIND;
 
     /* attempt native enabled port. */
-    sock = comm_openex(sock_type, proto, addr, flags, 0, 0, note);
+    sock = comm_openex(sock_type, proto, addr, flags, note);
 
     return sock;
 }
@@ -349,8 +349,6 @@ comm_openex(int sock_type,
             int proto,
             Ip::Address &addr,
             int flags,
-            tos_t tos,
-            nfmark_t nfmark,
             const char *note)
 {
     int new_socket;
@@ -408,14 +406,6 @@ comm_openex(int sock_type,
 
     debugs(50, 3, "comm_openex: Opened socket " << conn << " : family=" << AI->ai_family << ", type=" << AI->ai_socktype << ", protocol=" << AI->ai_protocol );
 
-    /* set TOS if needed */
-    if (tos)
-        Ip::Qos::setSockTos(conn, tos);
-
-    /* set netfilter mark if needed */
-    if (nfmark)
-        Ip::Qos::setSockNfmark(conn, nfmark);
-
     if ( Ip::EnableIpv6&IPV6_SPECIAL_SPLITSTACK && addr.isIPv6() )
         comm_set_v6only(conn->fd, 1);
 
@@ -424,7 +414,7 @@ comm_openex(int sock_type,
     if ( Ip::EnableIpv6&IPV6_SPECIAL_V4MAPPING && addr.isIPv6() )
         comm_set_v6only(conn->fd, 0);
 
-    comm_init_opened(conn, tos, nfmark, note, AI);
+    comm_init_opened(conn, note, AI);
     new_socket = comm_apply_flags(conn->fd, addr, flags, AI);
 
     Ip::Address::FreeAddrInfo(AI);
@@ -439,8 +429,6 @@ comm_openex(int sock_type,
 /// update FD tables after a local or remote (IPC) comm_openex();
 void
 comm_init_opened(const Comm::ConnectionPointer &conn,
-                 tos_t tos,
-                 nfmark_t nfmark,
                  const char *note,
                  struct addrinfo *AI)
 {
@@ -458,9 +446,6 @@ comm_init_opened(const Comm::ConnectionPointer &conn,
 
     fde *F = &fd_table[conn->fd];
     F->local_addr = conn->local;
-    F->tosToServer = tos;
-
-    F->nfmarkToServer = nfmark;
 
     F->sock_family = AI->ai_family;
 }
@@ -537,7 +522,7 @@ comm_import_opened(const Comm::ConnectionPointer &conn,
     assert(Comm::IsConnOpen(conn));
     assert(AI);
 
-    comm_init_opened(conn, 0, 0, note, AI);
+    comm_init_opened(conn, note, AI);
 
     if (!(conn->flags & COMM_NOCLOEXEC))
         fd_table[conn->fd].flags.close_on_exec = true;

diff --git a/src/comm.h b/src/comm.h
index 3046604959e5bca6847b09c95a71ae59192d87af..f491212c7ab86de76350e39d0a166e0918ff6056 100644 (file)
--- a/src/comm.h
+++ b/src/comm.h
@@ -51,7 +51,7 @@ void comm_import_opened(const Comm::ConnectionPointer &, const char *note, struc
 int comm_open_listener(int sock_type, int proto, Ip::Address &addr, int flags, const char *note);
 void comm_open_listener(int sock_type, int proto, Comm::ConnectionPointer &conn, const char *note);
 
-int comm_openex(int, int, Ip::Address &, int, tos_t tos, nfmark_t nfmark, const char *);
+int comm_openex(int, int, Ip::Address &, int, const char *);
 unsigned short comm_local_port(int fd);
 
 int comm_udp_sendto(int sock, const Ip::Address &to, const void *buf, int buflen);

diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
index 11ecb1421a2310f8ea5e743301039bbd2d2d8274..06cc08bd69a8bef51de3883f3437211290132e3a 100644 (file)
--- a/src/comm/ConnOpener.cc
+++ b/src/comm/ConnOpener.cc
@@ -14,6 +14,7 @@
 #include "icmp/net_db.h"
 #include "ip/tools.h"
 #include "ipcache.h"
+#include "ip/QosConfig.h"
 #include "SquidConfig.h"
 #include "SquidTime.h"
 
@@ -254,12 +255,25 @@ Comm::ConnOpener::createFd()
     if (callback_ == NULL || callback_->canceled())
         return false;
 
-    temporaryFd_ = comm_openex(SOCK_STREAM, IPPROTO_TCP, conn_->local, conn_->flags, conn_->tos, conn_->nfmark, host_);
+    temporaryFd_ = comm_openex(SOCK_STREAM, IPPROTO_TCP, conn_->local, conn_->flags, host_);
     if (temporaryFd_ < 0) {
         sendAnswer(Comm::ERR_CONNECT, 0, "Comm::ConnOpener::createFd");
         return false;
     }
 
+    // Set TOS if needed.
+    if (conn_->tos &&
+        Ip::Qos::setSockTos(temporaryFd_, conn_->tos, conn_->remote.isIPv4() ? AF_INET : AF_INET6) < 0)
+        conn_->tos = 0;
+#if SO_MARK
+    if (conn_->nfmark &&
+        Ip::Qos::setSockNfmark(temporaryFd_, conn_->nfmark) < 0)
+        conn_->nfmark = 0;
+#endif
+
+    fd_table[conn_->fd].tosToServer = conn_->tos;
+    fd_table[conn_->fd].nfmarkToServer = conn_->nfmark;
+
     typedef CommCbMemFunT<Comm::ConnOpener, CommCloseCbParams> abortDialer;
     calls_.earlyAbort_ = JobCallback(5, 4, abortDialer, this, Comm::ConnOpener::earlyAbort);
     comm_add_close_handler(temporaryFd_, calls_.earlyAbort_);

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index 244afb2484427e2eafafaafc59b0bcf69648fdcd..49b88c5d47b783e08ca55ea8fd8ff6c635ca7548 100644 (file)
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -47,6 +47,7 @@
 #include "fde.h"
 #include "globals.h"
 #include "ip/Intercept.h"
+#include "ip/QosConfig.h"
 #include "MasterXaction.h"
 #include "profiler/Profiler.h"
 #include "SquidConfig.h"
@@ -199,6 +200,21 @@ Comm::TcpAcceptor::setListen()
 #endif
     }
 
+#if 0
+    // Untested code.
+    // Set TOS if needed.
+    // To correctly implement TOS values on listening sockets, probably requires
+    // more work to inherit TOS values to created connection objects.
+    if (conn->tos &&
+        Ip::Qos::setSockTos(conn->fd, conn->tos, conn->remote.isIPv4() ? AF_INET : AF_INET6) < 0)
+        conn->tos = 0;
+#if SO_MARK
+    if (conn->nfmark &&
+        Ip::Qos::setSockNfmark(conn->fd, conn->nfmark) < 0)
+        conn->nfmark = 0;
+#endif
+#endif
+
     typedef CommCbMemFunT<Comm::TcpAcceptor, CommCloseCbParams> Dialer;
     closer_ = JobCallback(5, 4, Dialer, this, Comm::TcpAcceptor::handleClosure);
     comm_add_close_handler(conn->fd, closer_);

diff --git a/src/ftp.cc b/src/ftp.cc
index d7006ffd602b9a74c59e06ebe3c14fb47cf22221..1f8dd02ada13a307e536b7956c137888954edad3 100644 (file)
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -643,6 +643,9 @@ FtpStateData::listenForDataChannel(const Comm::ConnectionPointer &conn, const ch
         debugs(9, 3, HERE << "Unconnected data socket created on " << conn);
     }
 
+    conn->tos = ctrl.conn->tos;
+    conn->nfmark = ctrl.conn->nfmark;
+
     assert(Comm::IsConnOpen(conn));
     AsyncJob::Start(new Comm::TcpAcceptor(conn, note, sub));
 
@@ -2469,6 +2472,8 @@ ftpReadEPSV(FtpStateData* ftpState)
     conn->local.port(0);
     conn->remote = ftpState->ctrl.conn->remote;
     conn->remote.port(port);
+    conn->tos = ftpState->ctrl.conn->tos;
+    conn->nfmark = ftpState->ctrl.conn->nfmark;
 
     debugs(9, 3, HERE << "connecting to " << conn->remote);
 

diff --git a/src/ip/Qos.cci b/src/ip/Qos.cci
index c62f9345b00520b069fa49e3d99b02edc8f6012d..b95abc504f9281576ac22a133ffd907d1dcd27b6 100644 (file)
--- a/src/ip/Qos.cci
+++ b/src/ip/Qos.cci
@@ -3,7 +3,7 @@
 #include "Debug.h"
 
 int
-Ip::Qos::setSockTos(const Comm::ConnectionPointer &conn, tos_t tos)
+Ip::Qos::setSockTos(const int fd, tos_t tos, int type)
 {
     // Bug 3731: FreeBSD produces 'invalid option'
     // unless we pass it a 32-bit variable storing 8-bits of data.
@@ -11,26 +11,21 @@ Ip::Qos::setSockTos(const Comm::ConnectionPointer &conn, tos_t tos)
     //     so we convert to a int before setting.
     int bTos = tos;
 
-    if (conn->remote.isIPv4()) {
+    if (type == AF_INET) {
 #if defined(IP_TOS)
-        int x = setsockopt(conn->fd, IPPROTO_IP, IP_TOS, &bTos, sizeof(bTos));
+        const int x = setsockopt(fd, IPPROTO_IP, IP_TOS, &bTos, sizeof(bTos));
         if (x < 0)
-            debugs(50, 2, "Ip::Qos::setSockTos: setsockopt(IP_TOS) on " << conn << ": " << xstrerror());
-        else
-            conn->tos = tos;
+            debugs(50, 2, "Ip::Qos::setSockTos: setsockopt(IP_TOS) on " << fd << ": " << xstrerror());
         return x;
 #else
         debugs(50, DBG_IMPORTANT, "WARNING: setsockopt(IP_TOS) not supported on this platform");
         return -1;
 #endif
-
-    } else { // if (conn->remote.isIPv6()) {
+    } else { // type == AF_INET6
 #if defined(IPV6_TCLASS)
-        int x = setsockopt(conn->fd, IPPROTO_IPV6, IPV6_TCLASS, &bTos, sizeof(bTos));
+        const int x = setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &bTos, sizeof(bTos));
         if (x < 0)
-            debugs(50, 2, "Ip::Qos::setSockTos: setsockopt(IPV6_TCLASS) on " << conn << ": " << xstrerror());
-        else
-            conn->tos = tos;
+            debugs(50, 2, "Ip::Qos::setSockTos: setsockopt(IPV6_TCLASS) on " << fd << ": " << xstrerror());
         return x;
 #else
         debugs(50, DBG_IMPORTANT, "WARNING: setsockopt(IPV6_TCLASS) not supported on this platform");
@@ -42,14 +37,22 @@ Ip::Qos::setSockTos(const Comm::ConnectionPointer &conn, tos_t tos)
 }
 
 int
-Ip::Qos::setSockNfmark(const Comm::ConnectionPointer &conn, nfmark_t mark)
+Ip::Qos::setSockTos(const Comm::ConnectionPointer &conn, tos_t tos)
+{
+    const int x = Ip::Qos::setSockTos(conn->fd, tos, conn->remote.isIPv4() ? AF_INET : AF_INET6);
+    if (x >= 0)
+        conn->tos = tos;
+
+    return x;
+}
+
+int
+Ip::Qos::setSockNfmark(const int fd, nfmark_t mark)
 {
 #if SO_MARK && USE_LIBCAP
-    int x = setsockopt(conn->fd, SOL_SOCKET, SO_MARK, &mark, sizeof(nfmark_t));
+    const int x = setsockopt(fd, SOL_SOCKET, SO_MARK, &mark, sizeof(nfmark_t));
     if (x < 0)
-        debugs(50, 2, "setSockNfmark: setsockopt(SO_MARK) on " << conn << ": " << xstrerror());
-    else
-        conn->nfmark = mark;
+        debugs(50, 2, "setSockNfmark: setsockopt(SO_MARK) on " << fd << ": " << xstrerror());
     return x;
 #elif USE_LIBCAP
     debugs(50, DBG_IMPORTANT, "WARNING: setsockopt(SO_MARK) not supported on this platform");
@@ -60,6 +63,15 @@ Ip::Qos::setSockNfmark(const Comm::ConnectionPointer &conn, nfmark_t mark)
 #endif
 }
 
+int
+Ip::Qos::setSockNfmark(const Comm::ConnectionPointer &conn, nfmark_t mark)
+{
+    const int x = Ip::Qos::setSockNfmark(conn->fd, mark);
+    if (x >= 0)
+        conn->nfmark = mark;
+    return x;
+}
+
 bool
 Ip::Qos::Config::isHitTosActive() const
 {

diff --git a/src/ip/QosConfig.h b/src/ip/QosConfig.h
index 3229595acfa0ad2eb6b61ad8fe29909b87d7d729..c83d51c1864012f68720fb99f05bbd3f54ccc2b4 100644 (file)
--- a/src/ip/QosConfig.h
+++ b/src/ip/QosConfig.h
@@ -122,6 +122,14 @@ int doNfmarkLocalHit(const Comm::ConnectionPointer &conn);
 */
 _SQUID_INLINE_ int setSockTos(const Comm::ConnectionPointer &conn, tos_t tos);
 
+/**
+* The low level variant of setSockTos function to set TOS value of packets.
+* Avoid if you can use the Connection-based setSockTos().
+* @param fd Descriptor of socket to set the TOS for
+* @param type The socket family, AF_INET or AF_INET6
+*/
+_SQUID_INLINE_ int setSockTos(const int fd, tos_t tos, int type);
+
 /**
 * Function to set the netfilter mark value of packets. Sets the value on the
 * socket which then gets copied to the packets. Called from Ip::Qos::doNfmarkLocalMiss
@@ -129,6 +137,14 @@ _SQUID_INLINE_ int setSockTos(const Comm::ConnectionPointer &conn, tos_t tos);
 */
 _SQUID_INLINE_ int setSockNfmark(const Comm::ConnectionPointer &conn, nfmark_t mark);
 
+/**
+* The low level variant of setSockNfmark function to set the netfilter mark
+* value of packets.
+* Avoid if you can use the Connection-based setSockNfmark().
+* @param fd Descriptor of socket to set the mark for
+*/
+_SQUID_INLINE_ int setSockNfmark(const int fd, nfmark_t mark);
+
 /**
  * QOS configuration class. Contains all the parameters for QOS functions as well
  * as functions to check whether either TOS or MARK QOS is enabled.