Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Michal Kubecek
On Thu, Jul 16, 2015 at 10:50:59AM -0300, Marcelo Ricardo Leitner wrote:
> On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
> > @@ -278,6 +292,14 @@ static int sctp_new_state(enum ip_conntrack_dir dir,
> > pr_debug("SCTP_CID_SHUTDOWN_COMPLETE\n");
> > i = 8;
> > break;
> > +   case SCTP_CID_HEARTBEAT:
> > +   pr_debug("SCTP_CID_HEARTBEAT");
> > +   i = 9;
> > +   break;
> > +   case SCTP_CID_HEARTBEAT_ACK:
> > +   pr_debug("SCTP_CID_HEARTBEAT_ACK");
> > +   i = 10;
> > +   break;
> > default:
> > /* Other chunks like DATA, SACK, HEARTBEAT and
> > its ACK do not cause a change in state */
> 
> Would you update this comment on default case please? As with this
> patch, HB and its ACK may cause a change in state.

Thank you for catching this. I'll update the comment in v2 I'm going to
send tomorrow after some testing.

 Michal Kubecek

--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Marcelo Ricardo Leitner
On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
> Currently nf_conntrack_proto_sctp module handles only packets between
> primary addresses used to establish the connection. Any packets between
> secondary addresses are classified as invalid so that usual firewall
> configurations drop them. Allowing HEARTBEAT and HEARTBEAT-ACK chunks to
> establish a new conntrack would allow traffic between secondary
> addresses to pass through. A more sophisticated solution based on the
> addresses advertised in the initial handshake (and possibly also later
> dynamic address addition and removal) would be much harder to implement.
> Moreover, in general we cannot assume to always see the initial
> handshake as it can be routed through a different path.
> 
> The patch adds two new conntrack states:
> 
>   SCTP_CONNTRACK_HB_SENT  - a HEARTBEAT chunk seen but not acked
>   SCTP_CONNTRACK_HB_ACKED - a HEARTBEAT acked by HEARTBEAT-ACK
> 
> State transition rules:
> 
> - HB_SENT responds to usual chunks the same way as NONE (so that the
>   behaviour changes as little as possible)
> - HB_ACKED responds to usual chunks the same way as ESTABLISHED does,
>   except the resulting state is HB_ACKED rather than ESTABLISHED
> - previously existing states except NONE are preserved when HEARTBEAT or
>   HEARTBEAT-ACK is seen
> - NONE (in the initial direction) changes to HB_SENT on HEARTBEAT
>   and to CLOSED on HEARTBEAT-ACK
> - HB_SENT changes to HB_ACKED on HEARTBEAT-ACK in the reply direction
> - HB_SENT and HB_ACKED are preserved on HEARTBEAT/HEARTBEAT-ACK
>   otherwise
> 
> Default timeout values for new states are
> 
>   HB_SENT: 30 seconds (default hb_interval)
>   HB_ACKED: 210 seconds (hb_interval * path_max_retry + max_rto)
> 
> (We cannot expect to see the shutdown sequence so that the HB_ACKED
> timeout shouldn't be too long.)
> 
> Signed-off-by: Michal Kubecek 
> ---
>  include/uapi/linux/netfilter/nf_conntrack_sctp.h |   2 +
>  net/netfilter/nf_conntrack_proto_sctp.c  | 110 
> ++-
>  2 files changed, 90 insertions(+), 22 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h 
> b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> index ceeefe6681b5..3ec7a6082457 100644
> --- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> +++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> @@ -13,6 +13,8 @@ enum sctp_conntrack {
>   SCTP_CONNTRACK_SHUTDOWN_SENT,
>   SCTP_CONNTRACK_SHUTDOWN_RECD,
>   SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
> + SCTP_CONNTRACK_HB_SENT,
> + SCTP_CONNTRACK_HB_ACKED,
>   SCTP_CONNTRACK_MAX
>  };
>  
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c 
> b/net/netfilter/nf_conntrack_proto_sctp.c
> index b45da90fad32..efb6d5b16393 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -42,6 +42,8 @@ static const char *const sctp_conntrack_names[] = {
>   "SHUTDOWN_SENT",
>   "SHUTDOWN_RECD",
>   "SHUTDOWN_ACK_SENT",
> + "HEARTBEAT_SENT",
> + "HEARTBEAT_ACKED",
>  };
>  
>  #define SECS  * HZ
> @@ -57,6 +59,8 @@ static unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] 
> __read_mostly = {
>   [SCTP_CONNTRACK_SHUTDOWN_SENT]  = 300 SECS / 1000,
>   [SCTP_CONNTRACK_SHUTDOWN_RECD]  = 300 SECS / 1000,
>   [SCTP_CONNTRACK_SHUTDOWN_ACK_SENT]  = 3 SECS,
> + [SCTP_CONNTRACK_HB_SENT]= 30 SECS,
> + [SCTP_CONNTRACK_HB_ACKED]   = 210 SECS,
>  };
>  
>  #define sNO SCTP_CONNTRACK_NONE
> @@ -67,6 +71,8 @@ static unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] 
> __read_mostly = {
>  #define  sSS SCTP_CONNTRACK_SHUTDOWN_SENT
>  #define  sSR SCTP_CONNTRACK_SHUTDOWN_RECD
>  #define  sSA SCTP_CONNTRACK_SHUTDOWN_ACK_SENT
> +#define  sHS SCTP_CONNTRACK_HB_SENT
> +#define  sHA SCTP_CONNTRACK_HB_ACKED
>  #define  sIV SCTP_CONNTRACK_MAX
>  
>  /*
> @@ -88,6 +94,10 @@ SHUTDOWN_ACK_SENT - We have seen a SHUTDOWN_ACK chunk in 
> the direction opposite
>   to that of the SHUTDOWN chunk.
>  CLOSED- We have seen a SHUTDOWN_COMPLETE chunk in the direction 
> of
>   the SHUTDOWN chunk. Connection is closed.
> +HEARTBEAT_SENT- We have seen a HEARTBEAT in a new flow.
> +HEARTBEAT_ACKED   - We have seen a HEARTBEAT-ACK in the direction opposite to
> + that of the HEARTBEAT chunk. Secondary connection is
> + established.
>  */
>  
>  /* TODO
> @@ -97,36 +107,40 @@ CLOSED- We have seen a SHUTDOWN_COMPLETE 
> chunk in the direction of
>   - Check the error type in the reply dir before transitioning from
>  cookie echoed to closed.
>   - Sec 5.2.4 of RFC 2960
> - - Multi Homing support.
> + - Full Multi Homing support.
>  */
>  
>  /* SCTP conntrack state transitions */
> -static const u8 sctp_conntracks[2][9][SCTP_CONNTRACK_MAX] = {
> +static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
>   

Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Marcelo Ricardo Leitner
On Thu, Jul 16, 2015 at 02:05:12PM +0200, Michal Kubecek wrote:
> On Wed, Jul 15, 2015 at 05:35:08PM -0300, Marcelo Ricardo Leitner wrote:
> > Hi,
> > 
> > On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
> > > On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
> > > > Michal Kubecek  wrote:
> > > > > + case SCTP_CID_HEARTBEAT:
> > > > > + pr_debug("SCTP_CID_HEARTBEAT");
> > > > > + i = 9;
> > > > > + break;
> > > > > + case SCTP_CID_HEARTBEAT_ACK:
> > > > > + pr_debug("SCTP_CID_HEARTBEAT_ACK");
> > > > > + i = 10;
> > > > > + break;
> > > > >   default:
> > > > >   /* Other chunks like DATA, SACK, HEARTBEAT and
> > > > >   its ACK do not cause a change in state */
> > > > > @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
> > > > >   !test_bit(SCTP_CID_COOKIE_ECHO, map) &&
> > > > >   !test_bit(SCTP_CID_ABORT, map) &&
> > > > >   !test_bit(SCTP_CID_SHUTDOWN_ACK, map) &&
> > > > > + !test_bit(SCTP_CID_HEARTBEAT, map) &&
> > > > > + !test_bit(SCTP_CID_HEARTBEAT_ACK, map) &&
> > > > >   sh->vtag != ct->proto.sctp.vtag[dir]) {
> > > > >   pr_debug("Verification tag check failed\n");
> > > > >   goto out;
> > > > > @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
> > > > >   /* Sec 8.5.1 (D) */
> > > > >   if (sh->vtag != ct->proto.sctp.vtag[dir])
> > > > >   goto out_unlock;
> > > > > + } else if (sch->type == SCTP_CID_HEARTBEAT ||
> > > > > +sch->type == SCTP_CID_HEARTBEAT_ACK) {
> > > > > + if (ct->proto.sctp.vtag[dir] == 0) {
> > > > > + pr_debug("Setting vtag %x for dir %d\n",
> > > > > +  sh->vtag, dir);
> > > > > + ct->proto.sctp.vtag[dir] = sh->vtag;
> > > > 
> > > > Could you please elaborate on the [dir] == 0 test?
> > > > 
> > > > I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
> > > > needed for SCTP_CID_HEARTBEAT ?
> > > > 
> > > > We found a conntrack entry so shouldn't the vtag[dir] already be > 0?
> > > 
> > > Yes, you are right. This was originally intended to handle the case when
> > > a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
> > > such HEARTBEAT would be dropped anyway in current version.
> > 
> > And we have to keep the first vtag attempted because otherwise an
> > attacker could just probe for the right one until she gets a reply.
> > 
> > IOW, if a different vtag is attempted, we should drop it as the packet
> > doesn't belong to that association/conntrack entry.
> > 
> > As vtags are always != 0 in such case, that's a way to know if we
> > already have that information or not.
> > 
> > > On the other hand, an alternative would be
> > > 
> > >   } else if (sch->type == SCTP_CID_HEARTBEAT_ACK &&
> > >  ct->proto.sctp.vtag[dir] == 0) {
> > >   pr_debug("Setting vtag %x for dir %d\n",
> > >sh->vtag, dir);
> > >   ct->proto.sctp.vtag[dir] = sh->vtag;
> > >   } else if ((sch->type == SCTP_CID_HEARTBEAT ||
> > >   sch->type == SCTP_CID_HEARTBEAT_ACK) &&
> > >  sh->vtag != ct->proto.sctp.vtag[dir]) {
> > >   pr_debug("Verification tag check failed\n");
> > >   goto out_unlock;
> > >   }
> > > 
> > > I'm not sure it looks better.
> > 
> > Now it seems swapped, we should save the tag on HB and check on
> > HB_ACK only and would have to check against !dir entry. Like:
> 
> I forgot to include the explanation of vtag setting/checking logic into
> the commit message. It is supposed to work like this:
> 
> Normally, vtag is set from the INIT chunk for the reply direction and
> from the INIT-ACK chunk for the originating direction (i.e. each of
> these defines vtag value for the opposite direction). For secondary

Erf, indeed. I totally confused it and thought they would be equal on
both directions.

> conntracks, we can't rely on seeing INIT/INIT-ACK and even if we have
> seen them, we would need to connect two different conntracks. Therefore
> simplified logic is applied: vtag of first packet in each direction
> (HEARTBEAT in the originating and HEARTBEAT-ACK in reply direction) is
> saved and all following packets in that direction are compared with this
> saved value. While INIT and INIT-ACK define vtag for the opposite
> direction (that's where "!dir" comes from), vtags extracted from
> HEARTBEAT and HEARTBEAT-ACK are always for their direction. And we have
> to check vtags on packets with HEARTBEAT chunks as well because their
> vtags should match vtag of the first (set in sctp_new()).

Yes, that's pretty much it. Original 

Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Michal Kubecek
On Wed, Jul 15, 2015 at 05:35:08PM -0300, Marcelo Ricardo Leitner wrote:
> Hi,
> 
> On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
> > On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
> > > Michal Kubecek  wrote:
> > > > +   case SCTP_CID_HEARTBEAT:
> > > > +   pr_debug("SCTP_CID_HEARTBEAT");
> > > > +   i = 9;
> > > > +   break;
> > > > +   case SCTP_CID_HEARTBEAT_ACK:
> > > > +   pr_debug("SCTP_CID_HEARTBEAT_ACK");
> > > > +   i = 10;
> > > > +   break;
> > > > default:
> > > > /* Other chunks like DATA, SACK, HEARTBEAT and
> > > > its ACK do not cause a change in state */
> > > > @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
> > > > !test_bit(SCTP_CID_COOKIE_ECHO, map) &&
> > > > !test_bit(SCTP_CID_ABORT, map) &&
> > > > !test_bit(SCTP_CID_SHUTDOWN_ACK, map) &&
> > > > +   !test_bit(SCTP_CID_HEARTBEAT, map) &&
> > > > +   !test_bit(SCTP_CID_HEARTBEAT_ACK, map) &&
> > > > sh->vtag != ct->proto.sctp.vtag[dir]) {
> > > > pr_debug("Verification tag check failed\n");
> > > > goto out;
> > > > @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
> > > > /* Sec 8.5.1 (D) */
> > > > if (sh->vtag != ct->proto.sctp.vtag[dir])
> > > > goto out_unlock;
> > > > +   } else if (sch->type == SCTP_CID_HEARTBEAT ||
> > > > +  sch->type == SCTP_CID_HEARTBEAT_ACK) {
> > > > +   if (ct->proto.sctp.vtag[dir] == 0) {
> > > > +   pr_debug("Setting vtag %x for dir %d\n",
> > > > +sh->vtag, dir);
> > > > +   ct->proto.sctp.vtag[dir] = sh->vtag;
> > > 
> > > Could you please elaborate on the [dir] == 0 test?
> > > 
> > > I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
> > > needed for SCTP_CID_HEARTBEAT ?
> > > 
> > > We found a conntrack entry so shouldn't the vtag[dir] already be > 0?
> > 
> > Yes, you are right. This was originally intended to handle the case when
> > a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
> > such HEARTBEAT would be dropped anyway in current version.
> 
> And we have to keep the first vtag attempted because otherwise an
> attacker could just probe for the right one until she gets a reply.
> 
> IOW, if a different vtag is attempted, we should drop it as the packet
> doesn't belong to that association/conntrack entry.
> 
> As vtags are always != 0 in such case, that's a way to know if we
> already have that information or not.
> 
> > On the other hand, an alternative would be
> > 
> > } else if (sch->type == SCTP_CID_HEARTBEAT_ACK &&
> >ct->proto.sctp.vtag[dir] == 0) {
> > pr_debug("Setting vtag %x for dir %d\n",
> >  sh->vtag, dir);
> > ct->proto.sctp.vtag[dir] = sh->vtag;
> > } else if ((sch->type == SCTP_CID_HEARTBEAT ||
> > sch->type == SCTP_CID_HEARTBEAT_ACK) &&
> >sh->vtag != ct->proto.sctp.vtag[dir]) {
> > pr_debug("Verification tag check failed\n");
> > goto out_unlock;
> > }
> > 
> > I'm not sure it looks better.
> 
> Now it seems swapped, we should save the tag on HB and check on
> HB_ACK only and would have to check against !dir entry. Like:

I forgot to include the explanation of vtag setting/checking logic into
the commit message. It is supposed to work like this:

Normally, vtag is set from the INIT chunk for the reply direction and
from the INIT-ACK chunk for the originating direction (i.e. each of
these defines vtag value for the opposite direction). For secondary
conntracks, we can't rely on seeing INIT/INIT-ACK and even if we have
seen them, we would need to connect two different conntracks. Therefore
simplified logic is applied: vtag of first packet in each direction
(HEARTBEAT in the originating and HEARTBEAT-ACK in reply direction) is
saved and all following packets in that direction are compared with this
saved value. While INIT and INIT-ACK define vtag for the opposite
direction (that's where "!dir" comes from), vtags extracted from
HEARTBEAT and HEARTBEAT-ACK are always for their direction. And we have
to check vtags on packets with HEARTBEAT chunks as well because their
vtags should match vtag of the first (set in sctp_new()).

  Michal Kubecek

--
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  

Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Marcelo Ricardo Leitner
On Thu, Jul 16, 2015 at 02:05:12PM +0200, Michal Kubecek wrote:
 On Wed, Jul 15, 2015 at 05:35:08PM -0300, Marcelo Ricardo Leitner wrote:
  Hi,
  
  On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
   On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
Michal Kubecek mkube...@suse.cz wrote:
 + case SCTP_CID_HEARTBEAT:
 + pr_debug(SCTP_CID_HEARTBEAT);
 + i = 9;
 + break;
 + case SCTP_CID_HEARTBEAT_ACK:
 + pr_debug(SCTP_CID_HEARTBEAT_ACK);
 + i = 10;
 + break;
   default:
   /* Other chunks like DATA, SACK, HEARTBEAT and
   its ACK do not cause a change in state */
 @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
   !test_bit(SCTP_CID_COOKIE_ECHO, map) 
   !test_bit(SCTP_CID_ABORT, map) 
   !test_bit(SCTP_CID_SHUTDOWN_ACK, map) 
 + !test_bit(SCTP_CID_HEARTBEAT, map) 
 + !test_bit(SCTP_CID_HEARTBEAT_ACK, map) 
   sh-vtag != ct-proto.sctp.vtag[dir]) {
   pr_debug(Verification tag check failed\n);
   goto out;
 @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
   /* Sec 8.5.1 (D) */
   if (sh-vtag != ct-proto.sctp.vtag[dir])
   goto out_unlock;
 + } else if (sch-type == SCTP_CID_HEARTBEAT ||
 +sch-type == SCTP_CID_HEARTBEAT_ACK) {
 + if (ct-proto.sctp.vtag[dir] == 0) {
 + pr_debug(Setting vtag %x for dir %d\n,
 +  sh-vtag, dir);
 + ct-proto.sctp.vtag[dir] = sh-vtag;

Could you please elaborate on the [dir] == 0 test?

I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
needed for SCTP_CID_HEARTBEAT ?

We found a conntrack entry so shouldn't the vtag[dir] already be  0?
   
   Yes, you are right. This was originally intended to handle the case when
   a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
   such HEARTBEAT would be dropped anyway in current version.
  
  And we have to keep the first vtag attempted because otherwise an
  attacker could just probe for the right one until she gets a reply.
  
  IOW, if a different vtag is attempted, we should drop it as the packet
  doesn't belong to that association/conntrack entry.
  
  As vtags are always != 0 in such case, that's a way to know if we
  already have that information or not.
  
   On the other hand, an alternative would be
   
 } else if (sch-type == SCTP_CID_HEARTBEAT_ACK 
ct-proto.sctp.vtag[dir] == 0) {
 pr_debug(Setting vtag %x for dir %d\n,
  sh-vtag, dir);
 ct-proto.sctp.vtag[dir] = sh-vtag;
 } else if ((sch-type == SCTP_CID_HEARTBEAT ||
 sch-type == SCTP_CID_HEARTBEAT_ACK) 
sh-vtag != ct-proto.sctp.vtag[dir]) {
 pr_debug(Verification tag check failed\n);
 goto out_unlock;
 }
   
   I'm not sure it looks better.
  
  Now it seems swapped, we should save the tag on HB and check on
  HB_ACK only and would have to check against !dir entry. Like:
 
 I forgot to include the explanation of vtag setting/checking logic into
 the commit message. It is supposed to work like this:
 
 Normally, vtag is set from the INIT chunk for the reply direction and
 from the INIT-ACK chunk for the originating direction (i.e. each of
 these defines vtag value for the opposite direction). For secondary

Erf, indeed. I totally confused it and thought they would be equal on
both directions.

 conntracks, we can't rely on seeing INIT/INIT-ACK and even if we have
 seen them, we would need to connect two different conntracks. Therefore
 simplified logic is applied: vtag of first packet in each direction
 (HEARTBEAT in the originating and HEARTBEAT-ACK in reply direction) is
 saved and all following packets in that direction are compared with this
 saved value. While INIT and INIT-ACK define vtag for the opposite
 direction (that's where !dir comes from), vtags extracted from
 HEARTBEAT and HEARTBEAT-ACK are always for their direction. And we have
 to check vtags on packets with HEARTBEAT chunks as well because their
 vtags should match vtag of the first (set in sctp_new()).

Yes, that's pretty much it. Original code reads better here then.

Thanks,
Marcelo


--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Marcelo Ricardo Leitner
On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
 Currently nf_conntrack_proto_sctp module handles only packets between
 primary addresses used to establish the connection. Any packets between
 secondary addresses are classified as invalid so that usual firewall
 configurations drop them. Allowing HEARTBEAT and HEARTBEAT-ACK chunks to
 establish a new conntrack would allow traffic between secondary
 addresses to pass through. A more sophisticated solution based on the
 addresses advertised in the initial handshake (and possibly also later
 dynamic address addition and removal) would be much harder to implement.
 Moreover, in general we cannot assume to always see the initial
 handshake as it can be routed through a different path.
 
 The patch adds two new conntrack states:
 
   SCTP_CONNTRACK_HB_SENT  - a HEARTBEAT chunk seen but not acked
   SCTP_CONNTRACK_HB_ACKED - a HEARTBEAT acked by HEARTBEAT-ACK
 
 State transition rules:
 
 - HB_SENT responds to usual chunks the same way as NONE (so that the
   behaviour changes as little as possible)
 - HB_ACKED responds to usual chunks the same way as ESTABLISHED does,
   except the resulting state is HB_ACKED rather than ESTABLISHED
 - previously existing states except NONE are preserved when HEARTBEAT or
   HEARTBEAT-ACK is seen
 - NONE (in the initial direction) changes to HB_SENT on HEARTBEAT
   and to CLOSED on HEARTBEAT-ACK
 - HB_SENT changes to HB_ACKED on HEARTBEAT-ACK in the reply direction
 - HB_SENT and HB_ACKED are preserved on HEARTBEAT/HEARTBEAT-ACK
   otherwise
 
 Default timeout values for new states are
 
   HB_SENT: 30 seconds (default hb_interval)
   HB_ACKED: 210 seconds (hb_interval * path_max_retry + max_rto)
 
 (We cannot expect to see the shutdown sequence so that the HB_ACKED
 timeout shouldn't be too long.)
 
 Signed-off-by: Michal Kubecek mkube...@suse.cz
 ---
  include/uapi/linux/netfilter/nf_conntrack_sctp.h |   2 +
  net/netfilter/nf_conntrack_proto_sctp.c  | 110 
 ++-
  2 files changed, 90 insertions(+), 22 deletions(-)
 
 diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h 
 b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
 index ceeefe6681b5..3ec7a6082457 100644
 --- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
 +++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
 @@ -13,6 +13,8 @@ enum sctp_conntrack {
   SCTP_CONNTRACK_SHUTDOWN_SENT,
   SCTP_CONNTRACK_SHUTDOWN_RECD,
   SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
 + SCTP_CONNTRACK_HB_SENT,
 + SCTP_CONNTRACK_HB_ACKED,
   SCTP_CONNTRACK_MAX
  };
  
 diff --git a/net/netfilter/nf_conntrack_proto_sctp.c 
 b/net/netfilter/nf_conntrack_proto_sctp.c
 index b45da90fad32..efb6d5b16393 100644
 --- a/net/netfilter/nf_conntrack_proto_sctp.c
 +++ b/net/netfilter/nf_conntrack_proto_sctp.c
 @@ -42,6 +42,8 @@ static const char *const sctp_conntrack_names[] = {
   SHUTDOWN_SENT,
   SHUTDOWN_RECD,
   SHUTDOWN_ACK_SENT,
 + HEARTBEAT_SENT,
 + HEARTBEAT_ACKED,
  };
  
  #define SECS  * HZ
 @@ -57,6 +59,8 @@ static unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] 
 __read_mostly = {
   [SCTP_CONNTRACK_SHUTDOWN_SENT]  = 300 SECS / 1000,
   [SCTP_CONNTRACK_SHUTDOWN_RECD]  = 300 SECS / 1000,
   [SCTP_CONNTRACK_SHUTDOWN_ACK_SENT]  = 3 SECS,
 + [SCTP_CONNTRACK_HB_SENT]= 30 SECS,
 + [SCTP_CONNTRACK_HB_ACKED]   = 210 SECS,
  };
  
  #define sNO SCTP_CONNTRACK_NONE
 @@ -67,6 +71,8 @@ static unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] 
 __read_mostly = {
  #define  sSS SCTP_CONNTRACK_SHUTDOWN_SENT
  #define  sSR SCTP_CONNTRACK_SHUTDOWN_RECD
  #define  sSA SCTP_CONNTRACK_SHUTDOWN_ACK_SENT
 +#define  sHS SCTP_CONNTRACK_HB_SENT
 +#define  sHA SCTP_CONNTRACK_HB_ACKED
  #define  sIV SCTP_CONNTRACK_MAX
  
  /*
 @@ -88,6 +94,10 @@ SHUTDOWN_ACK_SENT - We have seen a SHUTDOWN_ACK chunk in 
 the direction opposite
   to that of the SHUTDOWN chunk.
  CLOSED- We have seen a SHUTDOWN_COMPLETE chunk in the direction 
 of
   the SHUTDOWN chunk. Connection is closed.
 +HEARTBEAT_SENT- We have seen a HEARTBEAT in a new flow.
 +HEARTBEAT_ACKED   - We have seen a HEARTBEAT-ACK in the direction opposite to
 + that of the HEARTBEAT chunk. Secondary connection is
 + established.
  */
  
  /* TODO
 @@ -97,36 +107,40 @@ CLOSED- We have seen a SHUTDOWN_COMPLETE 
 chunk in the direction of
   - Check the error type in the reply dir before transitioning from
  cookie echoed to closed.
   - Sec 5.2.4 of RFC 2960
 - - Multi Homing support.
 + - Full Multi Homing support.
  */
  
  /* SCTP conntrack state transitions */
 -static const u8 sctp_conntracks[2][9][SCTP_CONNTRACK_MAX] = {
 +static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
   {
  /*   ORIGINAL*/
 -/*  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA */
 -/* init

Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Michal Kubecek
On Wed, Jul 15, 2015 at 05:35:08PM -0300, Marcelo Ricardo Leitner wrote:
 Hi,
 
 On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
  On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
   Michal Kubecek mkube...@suse.cz wrote:
+   case SCTP_CID_HEARTBEAT:
+   pr_debug(SCTP_CID_HEARTBEAT);
+   i = 9;
+   break;
+   case SCTP_CID_HEARTBEAT_ACK:
+   pr_debug(SCTP_CID_HEARTBEAT_ACK);
+   i = 10;
+   break;
default:
/* Other chunks like DATA, SACK, HEARTBEAT and
its ACK do not cause a change in state */
@@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
!test_bit(SCTP_CID_COOKIE_ECHO, map) 
!test_bit(SCTP_CID_ABORT, map) 
!test_bit(SCTP_CID_SHUTDOWN_ACK, map) 
+   !test_bit(SCTP_CID_HEARTBEAT, map) 
+   !test_bit(SCTP_CID_HEARTBEAT_ACK, map) 
sh-vtag != ct-proto.sctp.vtag[dir]) {
pr_debug(Verification tag check failed\n);
goto out;
@@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
/* Sec 8.5.1 (D) */
if (sh-vtag != ct-proto.sctp.vtag[dir])
goto out_unlock;
+   } else if (sch-type == SCTP_CID_HEARTBEAT ||
+  sch-type == SCTP_CID_HEARTBEAT_ACK) {
+   if (ct-proto.sctp.vtag[dir] == 0) {
+   pr_debug(Setting vtag %x for dir %d\n,
+sh-vtag, dir);
+   ct-proto.sctp.vtag[dir] = sh-vtag;
   
   Could you please elaborate on the [dir] == 0 test?
   
   I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
   needed for SCTP_CID_HEARTBEAT ?
   
   We found a conntrack entry so shouldn't the vtag[dir] already be  0?
  
  Yes, you are right. This was originally intended to handle the case when
  a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
  such HEARTBEAT would be dropped anyway in current version.
 
 And we have to keep the first vtag attempted because otherwise an
 attacker could just probe for the right one until she gets a reply.
 
 IOW, if a different vtag is attempted, we should drop it as the packet
 doesn't belong to that association/conntrack entry.
 
 As vtags are always != 0 in such case, that's a way to know if we
 already have that information or not.
 
  On the other hand, an alternative would be
  
  } else if (sch-type == SCTP_CID_HEARTBEAT_ACK 
 ct-proto.sctp.vtag[dir] == 0) {
  pr_debug(Setting vtag %x for dir %d\n,
   sh-vtag, dir);
  ct-proto.sctp.vtag[dir] = sh-vtag;
  } else if ((sch-type == SCTP_CID_HEARTBEAT ||
  sch-type == SCTP_CID_HEARTBEAT_ACK) 
 sh-vtag != ct-proto.sctp.vtag[dir]) {
  pr_debug(Verification tag check failed\n);
  goto out_unlock;
  }
  
  I'm not sure it looks better.
 
 Now it seems swapped, we should save the tag on HB and check on
 HB_ACK only and would have to check against !dir entry. Like:

I forgot to include the explanation of vtag setting/checking logic into
the commit message. It is supposed to work like this:

Normally, vtag is set from the INIT chunk for the reply direction and
from the INIT-ACK chunk for the originating direction (i.e. each of
these defines vtag value for the opposite direction). For secondary
conntracks, we can't rely on seeing INIT/INIT-ACK and even if we have
seen them, we would need to connect two different conntracks. Therefore
simplified logic is applied: vtag of first packet in each direction
(HEARTBEAT in the originating and HEARTBEAT-ACK in reply direction) is
saved and all following packets in that direction are compared with this
saved value. While INIT and INIT-ACK define vtag for the opposite
direction (that's where !dir comes from), vtags extracted from
HEARTBEAT and HEARTBEAT-ACK are always for their direction. And we have
to check vtags on packets with HEARTBEAT chunks as well because their
vtags should match vtag of the first (set in sctp_new()).

  Michal Kubecek

--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-16 Thread Michal Kubecek
On Thu, Jul 16, 2015 at 10:50:59AM -0300, Marcelo Ricardo Leitner wrote:
 On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
  @@ -278,6 +292,14 @@ static int sctp_new_state(enum ip_conntrack_dir dir,
  pr_debug(SCTP_CID_SHUTDOWN_COMPLETE\n);
  i = 8;
  break;
  +   case SCTP_CID_HEARTBEAT:
  +   pr_debug(SCTP_CID_HEARTBEAT);
  +   i = 9;
  +   break;
  +   case SCTP_CID_HEARTBEAT_ACK:
  +   pr_debug(SCTP_CID_HEARTBEAT_ACK);
  +   i = 10;
  +   break;
  default:
  /* Other chunks like DATA, SACK, HEARTBEAT and
  its ACK do not cause a change in state */
 
 Would you update this comment on default case please? As with this
 patch, HB and its ACK may cause a change in state.

Thank you for catching this. I'll update the comment in v2 I'm going to
send tomorrow after some testing.

 Michal Kubecek

--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-15 Thread Marcelo Ricardo Leitner
Hi,

On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
> On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
> > Michal Kubecek  wrote:
> > > + case SCTP_CID_HEARTBEAT:
> > > + pr_debug("SCTP_CID_HEARTBEAT");
> > > + i = 9;
> > > + break;
> > > + case SCTP_CID_HEARTBEAT_ACK:
> > > + pr_debug("SCTP_CID_HEARTBEAT_ACK");
> > > + i = 10;
> > > + break;
> > >   default:
> > >   /* Other chunks like DATA, SACK, HEARTBEAT and
> > >   its ACK do not cause a change in state */
> > > @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
> > >   !test_bit(SCTP_CID_COOKIE_ECHO, map) &&
> > >   !test_bit(SCTP_CID_ABORT, map) &&
> > >   !test_bit(SCTP_CID_SHUTDOWN_ACK, map) &&
> > > + !test_bit(SCTP_CID_HEARTBEAT, map) &&
> > > + !test_bit(SCTP_CID_HEARTBEAT_ACK, map) &&
> > >   sh->vtag != ct->proto.sctp.vtag[dir]) {
> > >   pr_debug("Verification tag check failed\n");
> > >   goto out;
> > > @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
> > >   /* Sec 8.5.1 (D) */
> > >   if (sh->vtag != ct->proto.sctp.vtag[dir])
> > >   goto out_unlock;
> > > + } else if (sch->type == SCTP_CID_HEARTBEAT ||
> > > +sch->type == SCTP_CID_HEARTBEAT_ACK) {
> > > + if (ct->proto.sctp.vtag[dir] == 0) {
> > > + pr_debug("Setting vtag %x for dir %d\n",
> > > +  sh->vtag, dir);
> > > + ct->proto.sctp.vtag[dir] = sh->vtag;
> > 
> > Could you please elaborate on the [dir] == 0 test?
> > 
> > I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
> > needed for SCTP_CID_HEARTBEAT ?
> > 
> > We found a conntrack entry so shouldn't the vtag[dir] already be > 0?
> 
> Yes, you are right. This was originally intended to handle the case when
> a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
> such HEARTBEAT would be dropped anyway in current version.

And we have to keep the first vtag attempted because otherwise an
attacker could just probe for the right one until she gets a reply.

IOW, if a different vtag is attempted, we should drop it as the packet
doesn't belong to that association/conntrack entry.

As vtags are always != 0 in such case, that's a way to know if we
already have that information or not.

> On the other hand, an alternative would be
> 
>   } else if (sch->type == SCTP_CID_HEARTBEAT_ACK &&
>  ct->proto.sctp.vtag[dir] == 0) {
>   pr_debug("Setting vtag %x for dir %d\n",
>sh->vtag, dir);
>   ct->proto.sctp.vtag[dir] = sh->vtag;
>   } else if ((sch->type == SCTP_CID_HEARTBEAT ||
>   sch->type == SCTP_CID_HEARTBEAT_ACK) &&
>  sh->vtag != ct->proto.sctp.vtag[dir]) {
>   pr_debug("Verification tag check failed\n");
>   goto out_unlock;
>   }
> 
> I'm not sure it looks better.

Now it seems swapped, we should save the tag on HB and check on
HB_ACK only and would have to check against !dir entry. Like:

comments with // here are just informative on this email and may not
necessary in the code

} else if (sch->type == SCTP_CID_HEARTBEAT)
if (!sh->vtag || sh->vtag != ct->proto.sctp.vtag[!dir]) 
{
/* Invalid */
goto out_unlock;
}
else if (!ct->proto.sctp.vtag[!dir]) {
pr_debug("Setting vtag %x for dir %d\n",
 sh->vtag, dir);
// by saving it on !dir direction, we
// don't have to enable HB_ACK on that
// if() above of exceptions, just HB
// itself
ct->proto.sctp.vtag[!dir] = sh->vtag;
}
// fallthrough: no action because it's a vtag we
// have already seen
} else if (sch->type == SCTP_CID_HEARTBEAT_ACK) {
// it's guaranteed tht sh->vtag == 
ct->proto.sctp.vtag[dir]
// by the if() of the exceptions above, so we
// just have to check for vtag == 0
if (!sh->vtag) {
pr_debug("Verification tag check failed\n");
goto out_unlock;
}
// ok, it's hand-shaked, now save it on the
// other direction too so that further packets
// can pass that check
   

Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-15 Thread Pablo Neira Ayuso
On Tue, Jul 14, 2015 at 06:28:50PM +0200, Michal Kubecek wrote:
> On Tue, Jul 14, 2015 at 05:38:47PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
> > > @@ -658,6 +696,18 @@ static struct ctl_table sctp_sysctl_table[] = {
> > >   .mode   = 0644,
> > >   .proc_handler   = proc_dointvec_jiffies,
> > >   },
> > > + {
> > > + .procname   = "nf_conntrack_sctp_timeout_heartbeat_sent",
> > > + .maxlen = sizeof(unsigned int),
> > > + .mode   = 0644,
> > > + .proc_handler   = proc_dointvec_jiffies,
> > > + },
> > > + {
> > > + .procname   = "nf_conntrack_sctp_timeout_heartbeat_acked",
> > > + .maxlen = sizeof(unsigned int),
> > > + .mode   = 0644,
> > > + .proc_handler   = proc_dointvec_jiffies,
> > > + },
> > >   { }
> > >  };
> > >  
> > > @@ -705,6 +755,18 @@ static struct ctl_table sctp_compat_sysctl_table[] = 
> > > {
> > >   .mode   = 0644,
> > >   .proc_handler   = proc_dointvec_jiffies,
> > >   },
> > > + {
> > > + .procname   = "ip_conntrack_sctp_timeout_heartbeat_sent",
> > > + .maxlen = sizeof(unsigned int),
> > > + .mode   = 0644,
> > > + .proc_handler   = proc_dointvec_jiffies,
> > > + },
> > > + {
> > > + .procname   = "ip_conntrack_sctp_timeout_heartbeat_acked",
> > > + .maxlen = sizeof(unsigned int),
> > > + .mode   = 0644,
> > > + .proc_handler   = proc_dointvec_jiffies,
> > > + },
> > >   { }
> > 
> > I don't see the nla_policy updates for the netlink cttimeout
> > interface.
> 
> I didn't realize those were needed. It means adding two entries to
> sctp_timeout_nla_policy and two values to enum ctattr_timeout_sctp?

Exactly.
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-15 Thread Marcelo Ricardo Leitner
Hi,

On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
 On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
  Michal Kubecek mkube...@suse.cz wrote:
   + case SCTP_CID_HEARTBEAT:
   + pr_debug(SCTP_CID_HEARTBEAT);
   + i = 9;
   + break;
   + case SCTP_CID_HEARTBEAT_ACK:
   + pr_debug(SCTP_CID_HEARTBEAT_ACK);
   + i = 10;
   + break;
 default:
 /* Other chunks like DATA, SACK, HEARTBEAT and
 its ACK do not cause a change in state */
   @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
 !test_bit(SCTP_CID_COOKIE_ECHO, map) 
 !test_bit(SCTP_CID_ABORT, map) 
 !test_bit(SCTP_CID_SHUTDOWN_ACK, map) 
   + !test_bit(SCTP_CID_HEARTBEAT, map) 
   + !test_bit(SCTP_CID_HEARTBEAT_ACK, map) 
 sh-vtag != ct-proto.sctp.vtag[dir]) {
 pr_debug(Verification tag check failed\n);
 goto out;
   @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
 /* Sec 8.5.1 (D) */
 if (sh-vtag != ct-proto.sctp.vtag[dir])
 goto out_unlock;
   + } else if (sch-type == SCTP_CID_HEARTBEAT ||
   +sch-type == SCTP_CID_HEARTBEAT_ACK) {
   + if (ct-proto.sctp.vtag[dir] == 0) {
   + pr_debug(Setting vtag %x for dir %d\n,
   +  sh-vtag, dir);
   + ct-proto.sctp.vtag[dir] = sh-vtag;
  
  Could you please elaborate on the [dir] == 0 test?
  
  I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
  needed for SCTP_CID_HEARTBEAT ?
  
  We found a conntrack entry so shouldn't the vtag[dir] already be  0?
 
 Yes, you are right. This was originally intended to handle the case when
 a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
 such HEARTBEAT would be dropped anyway in current version.

And we have to keep the first vtag attempted because otherwise an
attacker could just probe for the right one until she gets a reply.

IOW, if a different vtag is attempted, we should drop it as the packet
doesn't belong to that association/conntrack entry.

As vtags are always != 0 in such case, that's a way to know if we
already have that information or not.

 On the other hand, an alternative would be
 
   } else if (sch-type == SCTP_CID_HEARTBEAT_ACK 
  ct-proto.sctp.vtag[dir] == 0) {
   pr_debug(Setting vtag %x for dir %d\n,
sh-vtag, dir);
   ct-proto.sctp.vtag[dir] = sh-vtag;
   } else if ((sch-type == SCTP_CID_HEARTBEAT ||
   sch-type == SCTP_CID_HEARTBEAT_ACK) 
  sh-vtag != ct-proto.sctp.vtag[dir]) {
   pr_debug(Verification tag check failed\n);
   goto out_unlock;
   }
 
 I'm not sure it looks better.

Now it seems swapped, we should save the tag on HB and check on
HB_ACK only and would have to check against !dir entry. Like:

comments with // here are just informative on this email and may not
necessary in the code

} else if (sch-type == SCTP_CID_HEARTBEAT)
if (!sh-vtag || sh-vtag != ct-proto.sctp.vtag[!dir]) 
{
/* Invalid */
goto out_unlock;
}
else if (!ct-proto.sctp.vtag[!dir]) {
pr_debug(Setting vtag %x for dir %d\n,
 sh-vtag, dir);
// by saving it on !dir direction, we
// don't have to enable HB_ACK on that
// if() above of exceptions, just HB
// itself
ct-proto.sctp.vtag[!dir] = sh-vtag;
}
// fallthrough: no action because it's a vtag we
// have already seen
} else if (sch-type == SCTP_CID_HEARTBEAT_ACK) {
// it's guaranteed tht sh-vtag == 
ct-proto.sctp.vtag[dir]
// by the if() of the exceptions above, so we
// just have to check for vtag == 0
if (!sh-vtag) {
pr_debug(Verification tag check failed\n);
goto out_unlock;
}
// ok, it's hand-shaked, now save it on the
// other direction too so that further packets
// can pass that check
// if there was a vtag crossed some when, we
// don't care as this one is now established.

Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-15 Thread Pablo Neira Ayuso
On Tue, Jul 14, 2015 at 06:28:50PM +0200, Michal Kubecek wrote:
 On Tue, Jul 14, 2015 at 05:38:47PM +0200, Pablo Neira Ayuso wrote:
  On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
   @@ -658,6 +696,18 @@ static struct ctl_table sctp_sysctl_table[] = {
 .mode   = 0644,
 .proc_handler   = proc_dointvec_jiffies,
 },
   + {
   + .procname   = nf_conntrack_sctp_timeout_heartbeat_sent,
   + .maxlen = sizeof(unsigned int),
   + .mode   = 0644,
   + .proc_handler   = proc_dointvec_jiffies,
   + },
   + {
   + .procname   = nf_conntrack_sctp_timeout_heartbeat_acked,
   + .maxlen = sizeof(unsigned int),
   + .mode   = 0644,
   + .proc_handler   = proc_dointvec_jiffies,
   + },
 { }
};

   @@ -705,6 +755,18 @@ static struct ctl_table sctp_compat_sysctl_table[] = 
   {
 .mode   = 0644,
 .proc_handler   = proc_dointvec_jiffies,
 },
   + {
   + .procname   = ip_conntrack_sctp_timeout_heartbeat_sent,
   + .maxlen = sizeof(unsigned int),
   + .mode   = 0644,
   + .proc_handler   = proc_dointvec_jiffies,
   + },
   + {
   + .procname   = ip_conntrack_sctp_timeout_heartbeat_acked,
   + .maxlen = sizeof(unsigned int),
   + .mode   = 0644,
   + .proc_handler   = proc_dointvec_jiffies,
   + },
 { }
  
  I don't see the nla_policy updates for the netlink cttimeout
  interface.
 
 I didn't realize those were needed. It means adding two entries to
 sctp_timeout_nla_policy and two values to enum ctattr_timeout_sctp?

Exactly.
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Michal Kubecek
On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
> Michal Kubecek  wrote:
> > +   case SCTP_CID_HEARTBEAT:
> > +   pr_debug("SCTP_CID_HEARTBEAT");
> > +   i = 9;
> > +   break;
> > +   case SCTP_CID_HEARTBEAT_ACK:
> > +   pr_debug("SCTP_CID_HEARTBEAT_ACK");
> > +   i = 10;
> > +   break;
> > default:
> > /* Other chunks like DATA, SACK, HEARTBEAT and
> > its ACK do not cause a change in state */
> > @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
> > !test_bit(SCTP_CID_COOKIE_ECHO, map) &&
> > !test_bit(SCTP_CID_ABORT, map) &&
> > !test_bit(SCTP_CID_SHUTDOWN_ACK, map) &&
> > +   !test_bit(SCTP_CID_HEARTBEAT, map) &&
> > +   !test_bit(SCTP_CID_HEARTBEAT_ACK, map) &&
> > sh->vtag != ct->proto.sctp.vtag[dir]) {
> > pr_debug("Verification tag check failed\n");
> > goto out;
> > @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
> > /* Sec 8.5.1 (D) */
> > if (sh->vtag != ct->proto.sctp.vtag[dir])
> > goto out_unlock;
> > +   } else if (sch->type == SCTP_CID_HEARTBEAT ||
> > +  sch->type == SCTP_CID_HEARTBEAT_ACK) {
> > +   if (ct->proto.sctp.vtag[dir] == 0) {
> > +   pr_debug("Setting vtag %x for dir %d\n",
> > +sh->vtag, dir);
> > +   ct->proto.sctp.vtag[dir] = sh->vtag;
> 
> Could you please elaborate on the [dir] == 0 test?
> 
> I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
> needed for SCTP_CID_HEARTBEAT ?
> 
> We found a conntrack entry so shouldn't the vtag[dir] already be > 0?

Yes, you are right. This was originally intended to handle the case when
a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
such HEARTBEAT would be dropped anyway in current version.

On the other hand, an alternative would be

} else if (sch->type == SCTP_CID_HEARTBEAT_ACK &&
   ct->proto.sctp.vtag[dir] == 0) {
pr_debug("Setting vtag %x for dir %d\n",
 sh->vtag, dir);
ct->proto.sctp.vtag[dir] = sh->vtag;
} else if ((sch->type == SCTP_CID_HEARTBEAT ||
sch->type == SCTP_CID_HEARTBEAT_ACK) &&
   sh->vtag != ct->proto.sctp.vtag[dir]) {
pr_debug("Verification tag check failed\n");
goto out_unlock;
}

I'm not sure it looks better.

Michal Kubecek
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Michal Kubecek
On Tue, Jul 14, 2015 at 05:38:47PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
> > @@ -658,6 +696,18 @@ static struct ctl_table sctp_sysctl_table[] = {
> > .mode   = 0644,
> > .proc_handler   = proc_dointvec_jiffies,
> > },
> > +   {
> > +   .procname   = "nf_conntrack_sctp_timeout_heartbeat_sent",
> > +   .maxlen = sizeof(unsigned int),
> > +   .mode   = 0644,
> > +   .proc_handler   = proc_dointvec_jiffies,
> > +   },
> > +   {
> > +   .procname   = "nf_conntrack_sctp_timeout_heartbeat_acked",
> > +   .maxlen = sizeof(unsigned int),
> > +   .mode   = 0644,
> > +   .proc_handler   = proc_dointvec_jiffies,
> > +   },
> > { }
> >  };
> >  
> > @@ -705,6 +755,18 @@ static struct ctl_table sctp_compat_sysctl_table[] = {
> > .mode   = 0644,
> > .proc_handler   = proc_dointvec_jiffies,
> > },
> > +   {
> > +   .procname   = "ip_conntrack_sctp_timeout_heartbeat_sent",
> > +   .maxlen = sizeof(unsigned int),
> > +   .mode   = 0644,
> > +   .proc_handler   = proc_dointvec_jiffies,
> > +   },
> > +   {
> > +   .procname   = "ip_conntrack_sctp_timeout_heartbeat_acked",
> > +   .maxlen = sizeof(unsigned int),
> > +   .mode   = 0644,
> > +   .proc_handler   = proc_dointvec_jiffies,
> > +   },
> > { }
> 
> I don't see the nla_policy updates for the netlink cttimeout
> interface.

I didn't realize those were needed. It means adding two entries to
sctp_timeout_nla_policy and two values to enum ctattr_timeout_sctp?
Or something else is also needed?

Michal Kubecek
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Pablo Neira Ayuso
On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
> @@ -658,6 +696,18 @@ static struct ctl_table sctp_sysctl_table[] = {
>   .mode   = 0644,
>   .proc_handler   = proc_dointvec_jiffies,
>   },
> + {
> + .procname   = "nf_conntrack_sctp_timeout_heartbeat_sent",
> + .maxlen = sizeof(unsigned int),
> + .mode   = 0644,
> + .proc_handler   = proc_dointvec_jiffies,
> + },
> + {
> + .procname   = "nf_conntrack_sctp_timeout_heartbeat_acked",
> + .maxlen = sizeof(unsigned int),
> + .mode   = 0644,
> + .proc_handler   = proc_dointvec_jiffies,
> + },
>   { }
>  };
>  
> @@ -705,6 +755,18 @@ static struct ctl_table sctp_compat_sysctl_table[] = {
>   .mode   = 0644,
>   .proc_handler   = proc_dointvec_jiffies,
>   },
> + {
> + .procname   = "ip_conntrack_sctp_timeout_heartbeat_sent",
> + .maxlen = sizeof(unsigned int),
> + .mode   = 0644,
> + .proc_handler   = proc_dointvec_jiffies,
> + },
> + {
> + .procname   = "ip_conntrack_sctp_timeout_heartbeat_acked",
> + .maxlen = sizeof(unsigned int),
> + .mode   = 0644,
> + .proc_handler   = proc_dointvec_jiffies,
> + },
>   { }

I don't see the nla_policy updates for the netlink cttimeout
interface.
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Florian Westphal
Michal Kubecek  wrote:
> + case SCTP_CID_HEARTBEAT:
> + pr_debug("SCTP_CID_HEARTBEAT");
> + i = 9;
> + break;
> + case SCTP_CID_HEARTBEAT_ACK:
> + pr_debug("SCTP_CID_HEARTBEAT_ACK");
> + i = 10;
> + break;
>   default:
>   /* Other chunks like DATA, SACK, HEARTBEAT and
>   its ACK do not cause a change in state */
> @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
>   !test_bit(SCTP_CID_COOKIE_ECHO, map) &&
>   !test_bit(SCTP_CID_ABORT, map) &&
>   !test_bit(SCTP_CID_SHUTDOWN_ACK, map) &&
> + !test_bit(SCTP_CID_HEARTBEAT, map) &&
> + !test_bit(SCTP_CID_HEARTBEAT_ACK, map) &&
>   sh->vtag != ct->proto.sctp.vtag[dir]) {
>   pr_debug("Verification tag check failed\n");
>   goto out;
> @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
>   /* Sec 8.5.1 (D) */
>   if (sh->vtag != ct->proto.sctp.vtag[dir])
>   goto out_unlock;
> + } else if (sch->type == SCTP_CID_HEARTBEAT ||
> +sch->type == SCTP_CID_HEARTBEAT_ACK) {
> + if (ct->proto.sctp.vtag[dir] == 0) {
> + pr_debug("Setting vtag %x for dir %d\n",
> +  sh->vtag, dir);
> + ct->proto.sctp.vtag[dir] = sh->vtag;

Could you please elaborate on the [dir] == 0 test?

I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
needed for SCTP_CID_HEARTBEAT ?

We found a conntrack entry so shouldn't the vtag[dir] already be > 0?
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Florian Westphal
Michal Kubecek mkube...@suse.cz wrote:
 + case SCTP_CID_HEARTBEAT:
 + pr_debug(SCTP_CID_HEARTBEAT);
 + i = 9;
 + break;
 + case SCTP_CID_HEARTBEAT_ACK:
 + pr_debug(SCTP_CID_HEARTBEAT_ACK);
 + i = 10;
 + break;
   default:
   /* Other chunks like DATA, SACK, HEARTBEAT and
   its ACK do not cause a change in state */
 @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
   !test_bit(SCTP_CID_COOKIE_ECHO, map) 
   !test_bit(SCTP_CID_ABORT, map) 
   !test_bit(SCTP_CID_SHUTDOWN_ACK, map) 
 + !test_bit(SCTP_CID_HEARTBEAT, map) 
 + !test_bit(SCTP_CID_HEARTBEAT_ACK, map) 
   sh-vtag != ct-proto.sctp.vtag[dir]) {
   pr_debug(Verification tag check failed\n);
   goto out;
 @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
   /* Sec 8.5.1 (D) */
   if (sh-vtag != ct-proto.sctp.vtag[dir])
   goto out_unlock;
 + } else if (sch-type == SCTP_CID_HEARTBEAT ||
 +sch-type == SCTP_CID_HEARTBEAT_ACK) {
 + if (ct-proto.sctp.vtag[dir] == 0) {
 + pr_debug(Setting vtag %x for dir %d\n,
 +  sh-vtag, dir);
 + ct-proto.sctp.vtag[dir] = sh-vtag;

Could you please elaborate on the [dir] == 0 test?

I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
needed for SCTP_CID_HEARTBEAT ?

We found a conntrack entry so shouldn't the vtag[dir] already be  0?
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Pablo Neira Ayuso
On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
 @@ -658,6 +696,18 @@ static struct ctl_table sctp_sysctl_table[] = {
   .mode   = 0644,
   .proc_handler   = proc_dointvec_jiffies,
   },
 + {
 + .procname   = nf_conntrack_sctp_timeout_heartbeat_sent,
 + .maxlen = sizeof(unsigned int),
 + .mode   = 0644,
 + .proc_handler   = proc_dointvec_jiffies,
 + },
 + {
 + .procname   = nf_conntrack_sctp_timeout_heartbeat_acked,
 + .maxlen = sizeof(unsigned int),
 + .mode   = 0644,
 + .proc_handler   = proc_dointvec_jiffies,
 + },
   { }
  };
  
 @@ -705,6 +755,18 @@ static struct ctl_table sctp_compat_sysctl_table[] = {
   .mode   = 0644,
   .proc_handler   = proc_dointvec_jiffies,
   },
 + {
 + .procname   = ip_conntrack_sctp_timeout_heartbeat_sent,
 + .maxlen = sizeof(unsigned int),
 + .mode   = 0644,
 + .proc_handler   = proc_dointvec_jiffies,
 + },
 + {
 + .procname   = ip_conntrack_sctp_timeout_heartbeat_acked,
 + .maxlen = sizeof(unsigned int),
 + .mode   = 0644,
 + .proc_handler   = proc_dointvec_jiffies,
 + },
   { }

I don't see the nla_policy updates for the netlink cttimeout
interface.
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Michal Kubecek
On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
 Michal Kubecek mkube...@suse.cz wrote:
  +   case SCTP_CID_HEARTBEAT:
  +   pr_debug(SCTP_CID_HEARTBEAT);
  +   i = 9;
  +   break;
  +   case SCTP_CID_HEARTBEAT_ACK:
  +   pr_debug(SCTP_CID_HEARTBEAT_ACK);
  +   i = 10;
  +   break;
  default:
  /* Other chunks like DATA, SACK, HEARTBEAT and
  its ACK do not cause a change in state */
  @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
  !test_bit(SCTP_CID_COOKIE_ECHO, map) 
  !test_bit(SCTP_CID_ABORT, map) 
  !test_bit(SCTP_CID_SHUTDOWN_ACK, map) 
  +   !test_bit(SCTP_CID_HEARTBEAT, map) 
  +   !test_bit(SCTP_CID_HEARTBEAT_ACK, map) 
  sh-vtag != ct-proto.sctp.vtag[dir]) {
  pr_debug(Verification tag check failed\n);
  goto out;
  @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
  /* Sec 8.5.1 (D) */
  if (sh-vtag != ct-proto.sctp.vtag[dir])
  goto out_unlock;
  +   } else if (sch-type == SCTP_CID_HEARTBEAT ||
  +  sch-type == SCTP_CID_HEARTBEAT_ACK) {
  +   if (ct-proto.sctp.vtag[dir] == 0) {
  +   pr_debug(Setting vtag %x for dir %d\n,
  +sh-vtag, dir);
  +   ct-proto.sctp.vtag[dir] = sh-vtag;
 
 Could you please elaborate on the [dir] == 0 test?
 
 I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
 needed for SCTP_CID_HEARTBEAT ?
 
 We found a conntrack entry so shouldn't the vtag[dir] already be  0?

Yes, you are right. This was originally intended to handle the case when
a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
such HEARTBEAT would be dropped anyway in current version.

On the other hand, an alternative would be

} else if (sch-type == SCTP_CID_HEARTBEAT_ACK 
   ct-proto.sctp.vtag[dir] == 0) {
pr_debug(Setting vtag %x for dir %d\n,
 sh-vtag, dir);
ct-proto.sctp.vtag[dir] = sh-vtag;
} else if ((sch-type == SCTP_CID_HEARTBEAT ||
sch-type == SCTP_CID_HEARTBEAT_ACK) 
   sh-vtag != ct-proto.sctp.vtag[dir]) {
pr_debug(Verification tag check failed\n);
goto out_unlock;
}

I'm not sure it looks better.

Michal Kubecek
--
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 nf-next] netfilter: nf_ct_sctp: minimal multihoming support

2015-07-14 Thread Michal Kubecek
On Tue, Jul 14, 2015 at 05:38:47PM +0200, Pablo Neira Ayuso wrote:
 On Tue, Jul 14, 2015 at 02:23:11PM +0200, Michal Kubecek wrote:
  @@ -658,6 +696,18 @@ static struct ctl_table sctp_sysctl_table[] = {
  .mode   = 0644,
  .proc_handler   = proc_dointvec_jiffies,
  },
  +   {
  +   .procname   = nf_conntrack_sctp_timeout_heartbeat_sent,
  +   .maxlen = sizeof(unsigned int),
  +   .mode   = 0644,
  +   .proc_handler   = proc_dointvec_jiffies,
  +   },
  +   {
  +   .procname   = nf_conntrack_sctp_timeout_heartbeat_acked,
  +   .maxlen = sizeof(unsigned int),
  +   .mode   = 0644,
  +   .proc_handler   = proc_dointvec_jiffies,
  +   },
  { }
   };
   
  @@ -705,6 +755,18 @@ static struct ctl_table sctp_compat_sysctl_table[] = {
  .mode   = 0644,
  .proc_handler   = proc_dointvec_jiffies,
  },
  +   {
  +   .procname   = ip_conntrack_sctp_timeout_heartbeat_sent,
  +   .maxlen = sizeof(unsigned int),
  +   .mode   = 0644,
  +   .proc_handler   = proc_dointvec_jiffies,
  +   },
  +   {
  +   .procname   = ip_conntrack_sctp_timeout_heartbeat_acked,
  +   .maxlen = sizeof(unsigned int),
  +   .mode   = 0644,
  +   .proc_handler   = proc_dointvec_jiffies,
  +   },
  { }
 
 I don't see the nla_policy updates for the netlink cttimeout
 interface.

I didn't realize those were needed. It means adding two entries to
sctp_timeout_nla_policy and two values to enum ctattr_timeout_sctp?
Or something else is also needed?

Michal Kubecek
--
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/