Re: [PATCH net] ipvs: fix rtnl_lock lockups caused by start_sync_thread

2018-04-09 Thread Simon Horman 
On Mon, Apr 09, 2018 at 04:53:22PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Apr 09, 2018 at 10:20:18AM +0300, Simon Horman wrote:
> > On Sat, Apr 07, 2018 at 03:50:47PM +0300, Julian Anastasov wrote:
> > > syzkaller reports for wrong rtnl_lock usage in sync code [1] and [2]
> > > 
> > > We have 2 problems in start_sync_thread if error path is
> > > taken, eg. on memory allocation error or failure to configure
> > > sockets for mcast group or addr/port binding:
> > > 
> > > 1. recursive locking: holding rtnl_lock while calling sock_release
> > > which in turn calls again rtnl_lock in ip_mc_drop_socket to leave
> > > the mcast group, as noticed by Florian Westphal. Additionally,
> > > sock_release can not be called while holding sync_mutex (ABBA
> > > deadlock).
> > > 
> > > 2. task hung: holding rtnl_lock while calling kthread_stop to
> > > stop the running kthreads. As the kthreads do the same to leave
> > > the mcast group (sock_release -> ip_mc_drop_socket -> rtnl_lock)
> > > they hang.
> > > 
> > > Fix the problems by calling rtnl_unlock early in the error path,
> > > now sock_release is called after unlocking both mutexes.
> > > 
> > > Problem 3 (task hung reported by syzkaller [2]) is variant of
> > > problem 2: use _trylock to prevent one user to call rtnl_lock and
> > > then while waiting for sync_mutex to block kthreads that execute
> > > sock_release when they are stopped by stop_sync_thread.
> > 
> > ...
> > 
> > > Reported-and-tested-by: 
> > > syzbot+a46d6abf9d56b1365...@syzkaller.appspotmail.com
> > > Reported-and-tested-by: 
> > > syzbot+5fe074c01b2032ce9...@syzkaller.appspotmail.com
> > > Fixes: e0b26cc997d5 ("ipvs: call rtnl_lock early")
> > > Signed-off-by: Julian Anastasov 
> > 
> > Signed-off-by: Simon Horman 
> > 
> > Pablo, could you take this directly into nf and then onwards to net?

Great, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: ebtables: don't attempt to allocate 0-sized compat array

2018-04-09 Thread Pablo Neira Ayuso 
On Wed, Apr 04, 2018 at 09:13:30PM +0200, Florian Westphal wrote:
> Dmitry reports 32bit ebtables on 64bit kernel got broken by
> a recent change that returns -EINVAL when ruleset has no entries.
> 
> ebtables however only counts user-defined chains, so for the
> initial table nentries will be 0.
> 
> Don't try to allocate the compat array in this case, as no user
> defined rules exist no rule will need 64bit translation.

Applied, thanks Florian.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ipvs: fix rtnl_lock lockups caused by start_sync_thread

2018-04-09 Thread Pablo Neira Ayuso 
On Mon, Apr 09, 2018 at 10:20:18AM +0300, Simon Horman wrote:
> On Sat, Apr 07, 2018 at 03:50:47PM +0300, Julian Anastasov wrote:
> > syzkaller reports for wrong rtnl_lock usage in sync code [1] and [2]
> > 
> > We have 2 problems in start_sync_thread if error path is
> > taken, eg. on memory allocation error or failure to configure
> > sockets for mcast group or addr/port binding:
> > 
> > 1. recursive locking: holding rtnl_lock while calling sock_release
> > which in turn calls again rtnl_lock in ip_mc_drop_socket to leave
> > the mcast group, as noticed by Florian Westphal. Additionally,
> > sock_release can not be called while holding sync_mutex (ABBA
> > deadlock).
> > 
> > 2. task hung: holding rtnl_lock while calling kthread_stop to
> > stop the running kthreads. As the kthreads do the same to leave
> > the mcast group (sock_release -> ip_mc_drop_socket -> rtnl_lock)
> > they hang.
> > 
> > Fix the problems by calling rtnl_unlock early in the error path,
> > now sock_release is called after unlocking both mutexes.
> > 
> > Problem 3 (task hung reported by syzkaller [2]) is variant of
> > problem 2: use _trylock to prevent one user to call rtnl_lock and
> > then while waiting for sync_mutex to block kthreads that execute
> > sock_release when they are stopped by stop_sync_thread.
> 
> ...
> 
> > Reported-and-tested-by: 
> > syzbot+a46d6abf9d56b1365...@syzkaller.appspotmail.com
> > Reported-and-tested-by: 
> > syzbot+5fe074c01b2032ce9...@syzkaller.appspotmail.com
> > Fixes: e0b26cc997d5 ("ipvs: call rtnl_lock early")
> > Signed-off-by: Julian Anastasov 
> 
> Signed-off-by: Simon Horman 
> 
> Pablo, could you take this directly into nf and then onwards to net?

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: nf_conntrack_sip: allow duplicate SDP expectations

2018-04-09 Thread Pablo Neira Ayuso 
On Tue, Apr 03, 2018 at 12:10:09PM +0200, Florian Westphal wrote:
> Callum Sinclair reported SIP IP Phone errors that he tracked down to
> such phones sending session descriptions for different media types but
> with same port numbers.
> 
> The expect core will only 'refresh' existing expectation if it is
> from same master AND same expectation class (media type).
> As expectation class is different, we get an error.
> 
> The SIP connection tracking code will then
> 
> 1). drop the SDP packet
> 2). if an rtp expectation was already installed successfully,
> error on rtcp expectation will cancel the rtp one.
> 
> Make the expect core report back to caller when the conflict is due
> to different expectation class and have SIP tracker ignore soft-error.

Applied, thanks Florian.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: fix CONFIG_NF_REJECT_IPV6=m link error

2018-04-09 Thread Arnd Bergmann 
On Mon, Apr 9, 2018 at 4:37 PM, Pablo Neira Ayuso  wrote:
> Hi Arnd,
>
> On Mon, Apr 09, 2018 at 12:53:12PM +0200, Arnd Bergmann wrote:
>> We get a new link error with CONFIG_NFT_REJECT_INET=y and 
>> CONFIG_NF_REJECT_IPV6=m
>
> I think we can update NFT_REJECT_INET so it depends on NFT_REJECT_IPV4
> and NFT_REJECT_IPV6. This doesn't allow here CONFIG_NFT_REJECT_INET=y
> and CONFIG_NF_REJECT_IPV6=m.
>
> I mean, just like we do with NFT_FIB_INET.

That can only work if NFT_REJECT_INET can be made a 'tristate' symbol
again, so that code gets built as a loadable module if
CONFIG_NF_REJECT_IPV6=m.

> BTW, I think this problem has been is not related to the recent patch,
> but something older that kbuild robot has triggered more easily for
> some reason?

02c7b25e5f54 is the one that turned NF_TABLES_INET into a 'bool'
symbol. NFT_REJECT depends on NF_TABLES_INET, so it used to
restricted to a loadable module with IPV6=m, but can now be
built-in, which causes that link error.

  Arnd
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: fix CONFIG_NF_REJECT_IPV6=m link error

2018-04-09 Thread Pablo Neira Ayuso 
Hi Arnd,

On Mon, Apr 09, 2018 at 12:53:12PM +0200, Arnd Bergmann wrote:
> We get a new link error with CONFIG_NFT_REJECT_INET=y and 
> CONFIG_NF_REJECT_IPV6=m

I think we can update NFT_REJECT_INET so it depends on NFT_REJECT_IPV4
and NFT_REJECT_IPV6. This doesn't allow here CONFIG_NFT_REJECT_INET=y
and CONFIG_NF_REJECT_IPV6=m.

I mean, just like we do with NFT_FIB_INET.

BTW, I think this problem has been is not related to the recent patch,
but something older that kbuild robot has triggered more easily for
some reason?

Thanks for your patch!
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index d3220b43c832..b48c57bb9aaf 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -601,7 +601,8 @@ config NFT_REJECT
 
 config NFT_REJECT_INET
 	depends on NF_TABLES_INET
-	default NFT_REJECT
+	depends on NFT_REJECT_IPV4
+	depends on NFT_REJECT_IPV6
 	tristate
 
 config NFT_COMPAT

[PATCH] netfilter: fix CONFIG_NF_REJECT_IPV6=m link error

2018-04-09 Thread Arnd Bergmann 
We get a new link error with CONFIG_NFT_REJECT_INET=y and 
CONFIG_NF_REJECT_IPV6=m
after larger parts of the nftables modules are linked together:

net/netfilter/nft_reject_inet.o: In function `nft_reject_inet_eval':
nft_reject_inet.c:(.text+0x17c): undefined reference to `nf_send_unreach6'
nft_reject_inet.c:(.text+0x190): undefined reference to `nf_send_reset6'

The problem is that with NF_TABLES_INET set, we implicitly try to use
the ipv6 version as well for NFT_REJECT, but when CONFIG_IPV6 is set to
a loadable module, it's impossible to reach that.

The best workaround I found is to express the above as a Kconfig
dependency, forcing NFT_REJECT itself to be 'm' in that particular
configuration.

Fixes: 02c7b25e5f54 ("netfilter: nf_tables: build-in filter chain type")
Signed-off-by: Arnd Bergmann 
---
 net/netfilter/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 704b3832dbad..44d8a55e9721 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -594,6 +594,7 @@ config NFT_QUOTA
 config NFT_REJECT
default m if NETFILTER_ADVANCED=n
tristate "Netfilter nf_tables reject support"
+   depends on !NF_TABLES_INET || (IPV6!=m || m)
help
  This option adds the "reject" expression that you can use to
  explicitly deny and notify via TCP reset/ICMP informational errors
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH nf-next v4] netfilter: nf_osf: nf_osf_ttl() and nf_osf_match()

2018-04-09 Thread Fernando Fernandez Mancera 
Added nf_osf_ttl() and nf_osf_match() into nf_osf.c in order to start the
nftables OSF implementation.

Signed-off-by: Fernando Fernandez Mancera 
---
 include/linux/netfilter/nf_osf.h  |  28 
 include/uapi/linux/netfilter/nf_osf.h |  97 ++
 include/uapi/linux/netfilter/xt_osf.h | 106 +++
 net/netfilter/Kconfig |   4 +
 net/netfilter/Makefile|   1 +
 net/netfilter/nf_osf.c| 241 ++
 net/netfilter/xt_osf.c| 202 +---
 7 files changed, 390 insertions(+), 289 deletions(-)
 create mode 100644 include/linux/netfilter/nf_osf.h
 create mode 100644 include/uapi/linux/netfilter/nf_osf.h
 create mode 100644 net/netfilter/nf_osf.c

diff --git a/include/linux/netfilter/nf_osf.h b/include/linux/netfilter/nf_osf.h
new file mode 100644
index ..46ae80b1d096
--- /dev/null
+++ b/include/linux/netfilter/nf_osf.h
@@ -0,0 +1,28 @@
+#include 
+
+/*
+ * Initial window size option state machine: multiple of mss, mtu or
+ * plain numeric value. Can also be made as plain numeric value which
+ * is not a multiple of specified value.
+ */
+enum nf_osf_window_size_options {
+   OSF_WSS_PLAIN   = 0,
+   OSF_WSS_MSS,
+   OSF_WSS_MTU,
+   OSF_WSS_MODULO,
+   OSF_WSS_MAX,
+};
+
+enum osf_fmatch_states {
+   /* Packet does not match the fingerprint */
+   FMATCH_WRONG = 0,
+   /* Packet matches the fingerprint */
+   FMATCH_OK,
+   /* Options do not match the fingerprint, but header does */
+   FMATCH_OPT_WRONG,
+};
+
+bool nf_osf_match(const struct sk_buff *skb, u_int8_t family,
+   int hooknum, struct net_device *in, struct net_device *out,
+   const struct nf_osf_info *info, struct net *net);
+
diff --git a/include/uapi/linux/netfilter/nf_osf.h 
b/include/uapi/linux/netfilter/nf_osf.h
new file mode 100644
index ..9164ea564bdd
--- /dev/null
+++ b/include/uapi/linux/netfilter/nf_osf.h
@@ -0,0 +1,97 @@
+#ifndef _NF_OSF_H
+#define _NF_OSF_H
+
+#define MAXGENRELEN 32
+
+#define NF_OSF_GENRE(1<<0)
+#define NF_OSF_TTL  (1<<1)
+#define NF_OSF_LOG  (1<<2)
+#define NF_OSF_INVERT   (1<<3)
+
+#define NF_OSF_LOGLEVEL_ALL 0   /* log all matched fingerprints */
+#define NF_OSF_LOGLEVEL_FIRST   1   /* log only the first matced fingerprint */
+#define NF_OSF_LOGLEVEL_ALL_KNOWN   2 /* do not log unknown packets */
+
+#define NF_OSF_TTL_TRUE 0   /* True ip and fingerprint TTL comparison */
+
+/* Do not compare ip and fingerprint TTL at all */
+#define NF_OSF_TTL_NOCHECK  2
+
+/*
+ * Wildcard MSS (kind of).
+ * It is used to implement a state machine for the different wildcard values
+ * of the MSS and window sizes.
+ */
+struct nf_osf_wc {
+   __u32   wc;
+   __u32   val;
+};
+
+/*
+ * This struct represents IANA options
+ * http://www.iana.org/assignments/tcp-parameters
+ */
+struct nf_osf_opt {
+   __u16   kind, length;
+   struct nf_osf_wcwc;
+};
+
+struct nf_osf_info {
+   chargenre[MAXGENRELEN];
+   __u32   len;
+   __u32   flags;
+   __u32   loglevel;
+   __u32   ttl;
+};
+
+struct nf_osf_user_finger {
+   struct nf_osf_wcwss;
+
+   __u8ttl, df;
+   __u16   ss, mss;
+   __u16   opt_num;
+
+   chargenre[MAXGENRELEN];
+   charversion[MAXGENRELEN];
+   charsubtype[MAXGENRELEN];
+
+   /* MAX_IPOPTLEN is maximum if all options are NOPs or EOLs */
+   struct nf_osf_opt   opt[MAX_IPOPTLEN];
+};
+
+struct nf_osf_finger {
+   struct rcu_head rcu_head;
+   struct list_headfinger_entry;
+   struct nf_osf_user_finger   finger;
+};
+
+struct nf_osf_nlmsg {
+   struct nf_osf_user_finger   f;
+   struct iphdrip;
+   struct tcphdr   tcp;
+};
+
+/* Defines for IANA option kinds */
+
+enum iana_options {
+   OSFOPT_EOL = 0, /* End of options */
+   OSFOPT_NOP, /* NOP */
+   OSFOPT_MSS, /* Maximum segment size */
+   OSFOPT_WSO, /* Window scale option */
+   OSFOPT_SACKP,   /* SACK permitted */
+   OSFOPT_SACK,/* SACK */
+   OSFOPT_ECHO,
+   OSFOPT_ECHOREPLY,
+   OSFOPT_TS,  /* Timestamp option */
+   OSFOPT_POCP,/* Partial Order Connection Permitted */
+   OSFOPT_POSP,/* Partial Order Service Profile */
+
+   /* Others are not used in the current OSF */
+   OSFOPT_EMPTY = 255,
+};
+
+bool nf_osf_match(const struct sk_buff *skb, u_int8_t family,
+   int hooknum, struct net_device *in, struct net_device *out,
+

Re: [PATCH net] ipvs: fix rtnl_lock lockups caused by start_sync_thread

2018-04-09 Thread Simon Horman 
On Sat, Apr 07, 2018 at 03:50:47PM +0300, Julian Anastasov wrote:
> syzkaller reports for wrong rtnl_lock usage in sync code [1] and [2]
> 
> We have 2 problems in start_sync_thread if error path is
> taken, eg. on memory allocation error or failure to configure
> sockets for mcast group or addr/port binding:
> 
> 1. recursive locking: holding rtnl_lock while calling sock_release
> which in turn calls again rtnl_lock in ip_mc_drop_socket to leave
> the mcast group, as noticed by Florian Westphal. Additionally,
> sock_release can not be called while holding sync_mutex (ABBA
> deadlock).
> 
> 2. task hung: holding rtnl_lock while calling kthread_stop to
> stop the running kthreads. As the kthreads do the same to leave
> the mcast group (sock_release -> ip_mc_drop_socket -> rtnl_lock)
> they hang.
> 
> Fix the problems by calling rtnl_unlock early in the error path,
> now sock_release is called after unlocking both mutexes.
> 
> Problem 3 (task hung reported by syzkaller [2]) is variant of
> problem 2: use _trylock to prevent one user to call rtnl_lock and
> then while waiting for sync_mutex to block kthreads that execute
> sock_release when they are stopped by stop_sync_thread.

...

> Reported-and-tested-by: syzbot+a46d6abf9d56b1365...@syzkaller.appspotmail.com
> Reported-and-tested-by: syzbot+5fe074c01b2032ce9...@syzkaller.appspotmail.com
> Fixes: e0b26cc997d5 ("ipvs: call rtnl_lock early")
> Signed-off-by: Julian Anastasov 

Signed-off-by: Simon Horman 

Pablo, could you take this directly into nf and then onwards to net?

...

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html