From c6f168c13172952969554e27714dbd7d6d01e268 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Mon, 21 Jul 2014 17:55:27 +0300 Subject: [PATCH] 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 --- src/FwdState.cc | 14 +++++++++++++ src/cache_cf.cc | 6 ++++++ src/cf.data.pre | 4 ++++ src/comm.cc | 27 ++++++------------------ src/comm.h | 2 +- src/comm/ConnOpener.cc | 16 +++++++++++++- src/comm/TcpAcceptor.cc | 16 ++++++++++++++ src/ftp.cc | 5 +++++ src/ip/Qos.cci | 46 ++++++++++++++++++++++++++--------------- src/ip/QosConfig.h | 16 ++++++++++++++ 10 files changed, 112 insertions(+), 40 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 9936d79bb8..bbedfe63bc 100644 --- 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 413379339f..aefc3bbe15 100644 --- 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 11603383ef..3915bbec47 100644 --- 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 34b0ceb171..735eab3236 100644 --- 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 3046604959..f491212c7a 100644 --- 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 11ecb1421a..06cc08bd69 100644 --- 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 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 244afb2484..49b88c5d47 100644 --- 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 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 d7006ffd60..1f8dd02ada 100644 --- 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 c62f9345b0..b95abc504f 100644 --- 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 3229595acf..c83d51c186 100644 --- 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. -- 2.47.2