Bug 183997 - route-to rule passes traffic when no targets are specified
Summary: route-to rule passes traffic when no targets are specified
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Gleb Smirnoff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-15 12:20 UTC by Kajetan Staszkiewicz
Modified: 2014-08-25 15:53 UTC (History)
1 user (show)

See Also:


Attachments
file.txt (3.65 KB, text/plain)
2013-11-15 12:20 UTC, Kajetan Staszkiewicz
no flags Details
Updated patch fixing the issue. (3.25 KB, patch)
2014-08-10 17:48 UTC, Kajetan Staszkiewicz
no flags Details | Diff
improved patch (2.63 KB, patch)
2014-08-11 08:56 UTC, Gleb Smirnoff
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kajetan Staszkiewicz 2013-11-15 12:20:00 UTC
When rule's redirection pool is empty, a packet still gets forwarded but no redirection occurs. In case of using route-to the packet leaving the router will have original target MAC address and thus will be received by the rotuer itself again if public and internal interfaces are on the same network. Due to no TTL decrease in pf, this situation causes a network loop stealing bandwidth and CPU time.

Redirection pool can be emptied by misconfiguration or by some automatic healthchecking tool which decides that all targets are dead and there is nobody to forward the traffic to.

Fix: Pass the status of pf_map_addr to pf_set_rt_ifp and then to pf_create_state. If the status shows that pf_map_addr has failed, abort creation of the state and try to delete the src_node it could have created.

The attached patch is for FreeBSD 10.0-BETA3 and was not yet tested under some bigger load, although the same approach works well for FreeBSD 9.1. I can provide the 9.1 patch too if requested.

Patch attached with submission follows:
How-To-Repeat: Empty a table and have some traffic hit a route-to rule using this table.
Comment 1 Gleb Smirnoff freebsd_committer 2013-12-25 01:34:05 UTC
Responsible Changed
From-To: freebsd-bugs->glebius

Grab to not forget.
Comment 2 Kajetan Staszkiewicz 2014-08-10 17:48:47 UTC
Created attachment 145617 [details]
Updated patch fixing the issue.

Same thing as the previous patch but moved PFRES_MAPFAILED before PFRES_MAX, fixed enum for printing "map-failed" counter. Patch extracted from 10-STABLE sources.
Comment 3 Gleb Smirnoff freebsd_committer 2014-08-11 08:35:16 UTC
Thanks! I will commit patch to head as is, but for stable/10 we have requirement not to change the ABI, and we can't grow struct pf_status. Thus, for stable/10 we will probably account this as PFRES_STATEINS.
Comment 4 Gleb Smirnoff freebsd_committer 2014-08-11 08:56:28 UTC
Created attachment 145653 [details]
improved patch

Actually we can cut quite a lot of code here. Can you please review/test the attached patch?
Comment 5 commit-hook freebsd_committer 2014-08-15 14:03:07 UTC
A commit references this bug:

Author: glebius
Date: Fri Aug 15 14:02:25 UTC 2014
New revision: 270022
URL: http://svnweb.freebsd.org/changeset/base/270022

Log:
  pf_map_addr() can fail and in this case we should drop the packet,
  otherwise bad consequences including a routing loop can occur.

  Move pf_set_rt_ifp() earlier in state creation sequence and
  inline it, cutting some extra code.

  PR:		183997
  Submitted by:	Kajetan Staszkiewicz <vegeta tuxpowered.net>
  Sponsored by:	InnoGames GmbH

Changes:
  head/sys/netpfil/pf/pf.c
  head/sys/netpfil/pf/pf.h
Comment 6 commit-hook freebsd_committer 2014-08-25 15:50:26 UTC
A commit references this bug:

Author: glebius
Date: Mon Aug 25 15:49:41 UTC 2014
New revision: 270576
URL: http://svnweb.freebsd.org/changeset/base/270576

Log:
  Merge r270022 from head:
    pf_map_addr() can fail and in this case we should drop the packet,
    otherwise bad consequences including a routing loop can occur.

    Move pf_set_rt_ifp() earlier in state creation sequence and
    inline it, cutting some extra code.

    PR:           183997
    Submitted by: Kajetan Staszkiewicz <vegeta tuxpowered.net>
    Sponsored by: InnoGames GmbH

Changes:
_U  stable/10/
  stable/10/sys/netpfil/pf/pf.c
  stable/10/sys/netpfil/pf/pf.h