Re: [patch v2.2 1/4] [PATCH v2.1 1/4] netfilter: xt_ipvs (netfilter matcher for IPVS)

Re: [patch v2.2 1/4] [PATCH v2.1 1/4] netfilter: xt_ipvs (netfilter matcher for IPVS)

[Patrick McHardy]

Simon Horman wrote:

> @@ -0,0 +1,25 @@
> +#ifndef _XT_IPVS_H
> +#define _XT_IPVS_H 1

You don't need to define a value.

> +config NETFILTER_XT_MATCH_IPVS
> +	tristate '"ipvs" match support'
> +	depends on IP_VS
> +	depends on NETFILTER_ADVANCED
> +	help
> +	  This option allows you to match against IPVS properties of a packet.
> +
> +	  If unsure, say N.

You're using conntrack symbols, so this seems to need a dependency
on NF_CONNTRACK.

> +static bool ipvs_mt_check(const struct xt_mtchk_param *par)

We've changed the signature to "int" in nf-next to be able to
return errno codes. Please rebase your patches onto nf-next-2.6.git.

Please also CC netfilter-devel at least for those parts that affect
non-IPVS netfilter.

> +{
> +	if (par->family != NFPROTO_IPV4
> +#ifdef CONFIG_IP_VS_IPV6
> +	    && par->family != NFPROTO_IPV6
> +#endif
> +		) {
> +		pr_info("protocol family %u not supported\n", par->family);
> +		return false;
> +	}
> +
> +	return true;
> +}

Re: [patch v2.2 2/4] [PATCH v2.1 2/4] IPVS: make friends with nf_conntrack

[Patrick McHardy]

Looks good to me.

Re: [patch v2.2 3/4] [PATCH v2.1 3/4] IPVS: make FTP work with full NAT support

[Patrick McHardy]

Simon Horman wrote:

> +#define FMT_TUPLE	"%u.%u.%u.%u:%u->%u.%u.%u.%u:%u/%u"
> +#define ARG_TUPLE(T)	NIPQUAD((T)->src.u3.ip), ntohs((T)->src.u.all), \
> +			NIPQUAD((T)->dst.u3.ip), ntohs((T)->dst.u.all), \
> +			(T)->dst.protonum
> +
> +#define FMT_CONN	"%u.%u.%u.%u:%u->%u.%u.%u.%u:%u->%u.%u.%u.%u:%u/%u:%u"
> +#define ARG_CONN(C)	NIPQUAD((C)->caddr), ntohs((C)->cport), \
> +			NIPQUAD((C)->vaddr), ntohs((C)->vport), \
> +			NIPQUAD((C)->daddr), ntohs((C)->dport), \
> +			(C)->protocol, (C)->state
>  

Please use the appropriate format string (%pI4) instead of NIPQUAD.

> +		buf_len = sprintf(buf, "%u,%u,%u,%u,%u,%u", NIPQUAD(from.ip),
> +				  (ntohs(port)>>8)&255, ntohs(port)&255);
> +
> +		ct = nf_ct_get(skb, &ctinfo);
> +		ret = nf_nat_mangle_tcp_packet(skb,
> +					       ct,
> +					       ctinfo,
> +					       start-data,
> +					       end-start,
> +					       buf,
> +					       buf_len);
> +
> +		if (ct && ct != &nf_conntrack_untracked)

ct is non-NULL, otherwise we'll crash in nf_nat_mangle_tcp_packet().
Are you sure you want to mangle untracked packets above? That doesn't
work when their are size changes.

Re: [patch v2.2 1/4] [PATCH v2.1 1/4] netfilter: xt_ipvs (netfilter matcher for IPVS)

[Hannes Eder]

Thank you for picking this series of patches up again and thanks for
the feedback.

I'll send an updated version in the next days.

Cheers, -Hannes

On Sat, May 1, 2010 at 18:18, Patrick McHardy <kaber@trash.net> wrote:
> Simon Horman wrote:
>
>> @@ -0,0 +1,25 @@
>> +#ifndef _XT_IPVS_H
>> +#define _XT_IPVS_H 1
>
> You don't need to define a value.
>
>> +config NETFILTER_XT_MATCH_IPVS
>> +     tristate '"ipvs" match support'
>> +     depends on IP_VS
>> +     depends on NETFILTER_ADVANCED
>> +     help
>> +       This option allows you to match against IPVS properties of a packet.
>> +
>> +       If unsure, say N.
>
> You're using conntrack symbols, so this seems to need a dependency
> on NF_CONNTRACK.
>
>> +static bool ipvs_mt_check(const struct xt_mtchk_param *par)
>
> We've changed the signature to "int" in nf-next to be able to
> return errno codes. Please rebase your patches onto nf-next-2.6.git.
>
> Please also CC netfilter-devel at least for those parts that affect
> non-IPVS netfilter.
>
>> +{
>> +     if (par->family != NFPROTO_IPV4
>> +#ifdef CONFIG_IP_VS_IPV6
>> +         && par->family != NFPROTO_IPV6
>> +#endif
>> +             ) {
>> +             pr_info("protocol family %u not supported\n", par->family);
>> +             return false;
>> +     }
>> +
>> +     return true;
>> +}
>
>

Re: [patch v2.2 1/4] [PATCH v2.1 1/4] netfilter: xt_ipvs (netfilter matcher for IPVS)

[Simon Horman]

On Mon, May 03, 2010 at 01:29:46PM +0200, Hannes Eder wrote:
> Thank you for picking this series of patches up again and thanks for
> the feedback.
> 
> I'll send an updated version in the next days.

Hi Hanes,

more than a few days seems to have passed.
Do you have time to fix the patches up?
If not, I'll take a stab at it.

Thanks

Re: [patch v2.2 1/4] [PATCH v2.1 1/4] netfilter: xt_ipvs (netfilter matcher for IPVS)

[Hannes Eder]

Hi Simon,

On Tue, Jun 22, 2010 at 09:13, Simon Horman <horms@verge.net.au> wrote:
> On Mon, May 03, 2010 at 01:29:46PM +0200, Hannes Eder wrote:
>> Thank you for picking this series of patches up again and thanks for
>> the feedback.
>>
>> I'll send an updated version in the next days.
>
> Hi Hanes,
>
> more than a few days seems to have passed.
> Do you have time to fix the patches up?
> If not, I'll take a stab at it.

/me working through the backlog of emails after vacation, however this
email was buried in my inbox before my vacation, my bad.  I've been
extremely busy lately and I did not have the time to work on the
patches.  I saw your updated versions, I appreciate very much that you
are taking it from there.

Cheers,
-Hannes

Re: [patch v2.2 1/4] [PATCH v2.1 1/4] netfilter: xt_ipvs (netfilter matcher for IPVS)

[Simon Horman]

On Tue, Jul 20, 2010 at 02:44:11PM +0200, Hannes Eder wrote:
> Hi Simon,
> 
> On Tue, Jun 22, 2010 at 09:13, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, May 03, 2010 at 01:29:46PM +0200, Hannes Eder wrote:
> >> Thank you for picking this series of patches up again and thanks for
> >> the feedback.
> >>
> >> I'll send an updated version in the next days.
> >
> > Hi Hanes,
> >
> > more than a few days seems to have passed.
> > Do you have time to fix the patches up?
> > If not, I'll take a stab at it.
> 
> /me working through the backlog of emails after vacation, however this
> email was buried in my inbox before my vacation, my bad.  I've been
> extremely busy lately and I did not have the time to work on the
> patches.  I saw your updated versions, I appreciate very much that you
> are taking it from there.

No problem, I assumed that you were busy with something.