| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Pine.GSO.4.63.1208230008220.5113@stinky-local.trash.net>
Date: Thu, 23 Aug 2012 00:13:57 +0200 (MEST)
From: Patrick McHardy <kaber@...sh.net>
To: Jan Engelhardt <jengelh@...i.de>
cc: netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 06/18] netfilter: add protocol independant NAT core
On Mon, 20 Aug 2012, Jan Engelhardt wrote:
> On Monday 2012-08-20 05:39, Patrick McHardy wrote:
>>
>> enum ctattr_nat {
>> CTA_NAT_UNSPEC,
>> - CTA_NAT_MINIP,
>> - CTA_NAT_MAXIP,
>> + CTA_NAT_V4_MINIP,
>> +#define CTA_NAT_MINIP CTA_NAT_V4_MINIP
>> + CTA_NAT_V4_MAXIP,
>> +#define CTA_NAT_MAXIP CTA_NAT_V4_MAXIP
>> CTA_NAT_PROTO,
>> __CTA_NAT_MAX
>> };
>
> One could also
>
> enum ctattr_nat {
> ...
> __CTA_NAT_MAX,
>
> CTA_NAT_MINIP = CTA_NAT_V4_MINIP,
> CTA_NAT_MAXIP = CTA_NAT_V4_MAXIP,
> };
>
> to provide the old names.
Sure. Doesn't really matter since defines are not used consistently
(in which case you could use them for #ifdefs).
>> diff --git a/net/ipv4/netfilter/Kconfig b/net/ipv4/netfilter/Kconfig
>> index fcc543c..33372a1 100644
>> --- a/net/ipv4/netfilter/Kconfig
>> +++ b/net/ipv4/netfilter/Kconfig
> [...]
>>
>> -config NF_NAT_NEEDED
>> - bool
>> - depends on NF_NAT
>> - default y
>> -
>
> Could add "if NF_NAT_IPV4".."endif" block as appropriate around here,
> to save on all the extra "depend on NF_NAT_IPV4" clauses.
Agreed.
>> +static int nf_nat_ipv4_in_range(const struct nf_conntrack_tuple *t,
>> + const struct nf_nat_range *range)
>> +{
>> + return ntohl(t->src.u3.ip) >= ntohl(range->min_addr.ip) &&
>> + ntohl(t->src.u3.ip) <= ntohl(range->max_addr.ip);
>> +}
>
> static bool ..
Changed.
>> +static bool nf_nat_ipv4_manip_pkt(struct sk_buff *skb,
>> + unsigned int iphdroff,
>> + const struct nf_nat_l4proto *l4proto,
>> + const struct nf_conntrack_tuple *target,
>> + enum nf_nat_manip_type maniptype)
>> +{
>> + struct iphdr *iph;
>> + unsigned int hdroff;
>> +
>> + if (!skb_make_writable(skb, iphdroff + sizeof(*iph)))
>> + return false;
>> +
>> + iph = (void *)skb->data + iphdroff;
>
> Is iph = ip_hdr(skb), hdroff = iphdroff+skb_iphdrlen(iph) not usable here?
No, we're also translating the inner packets in ICMP error messages.
>> + hdroff = iphdroff + iph->ihl * 4;
>> +
>> + if (!l4proto->manip_pkt(skb, &nf_nat_l3proto_ipv4, iphdroff, hdroff,
>> + target, maniptype))
>> + return false;
>> + iph = (void *)skb->data + iphdroff;
>
> Is trying to avoid some GNU extensions a worthwhile goal? If so,
> iph = (struct iphdr *)(skb->data + iphdroff) should be used, like in:
I don't get your point.
>> +static void nf_nat_ipv4_csum_update(struct sk_buff *skb,
>> + unsigned int iphdroff, __sum16 *check,
>> + const struct nf_conntrack_tuple *t,
>> + enum nf_nat_manip_type maniptype)
>> +{
>> + struct iphdr *iph = (struct iphdr *)(skb->data + iphdroff);
>> [...]
>> +}
>
>
>
>> +static void nf_nat_ipv4_csum_recalc(struct sk_buff *skb,
>> + u8 proto, void *data, __sum16 *check,
>> + int datalen, int oldlen)
>> +{
>> + const struct iphdr *iph = ip_hdr(skb);
>> + struct rtable *rt = skb_rtable(skb);
>> +
>> + if (skb->ip_summed != CHECKSUM_PARTIAL) {
>> + if (!(rt->rt_flags & RTCF_LOCAL) &&
>> + (!skb->dev || skb->dev->features & NETIF_F_V4_CSUM)) {
>> + skb->ip_summed = CHECKSUM_PARTIAL;
>> + skb->csum_start = skb_headroom(skb) +
>> + skb_network_offset(skb) +
>> + ip_hdrlen(skb);
>> + skb->csum_offset = (void *)check - data;
>> + *check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
>> + datalen, proto, 0);
>> + } else {
>> + *check = 0;
>> + *check = csum_tcpudp_magic(iph->saddr, iph->daddr,
>> + datalen, proto,
>> + csum_partial(data, datalen,
>> + 0));
>> + if (proto == IPPROTO_UDP && !*check)
>> + *check = CSUM_MANGLED_0;
>> + }
>> + } else
>> + inet_proto_csum_replace2(check, skb,
>> + htons(oldlen), htons(datalen), 1);
>> +}
>
> Here is a style factory trick: invert the condition such that the
> simple case is first, and the big one becomes an else if
> with a reduced indent:
This is existing code, I don't want to bloat the diff by unnecessarily
rearranging it.
>> +static void __exit nf_nat_l3proto_ipv4_exit(void)
>> +{
>> + nf_nat_l3proto_unregister(&nf_nat_l3proto_ipv4);
>> + nf_nat_l4proto_unregister(NFPROTO_IPV4, &nf_nat_l4proto_icmp);
>> +}
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("nf-nat-" __stringify(AF_INET));
>
> Technically, this would have to be NFPROTO_IPV4, though GNU C has yet
> to gain an extension to stringify enum constants..
I'm aware of that.
>> + /* 1) If this srcip/proto/src-proto-part is currently mapped,
>> + * and that same mapping gives a unique tuple within the given
>> + * range, use that.
>> + *
>> + * This is only required for source (ie. NAT/masq) mappings.
>> + * So far, we don't do local source mappings, so multiple
>> + * manips not an issue.
> manips are not an issue.
>
>
>> - /* nf_conntrack_alter_reply might re-allocate extension area */
>> + /* nf_conntrack_alter_reply might re-allocate exntension aera */
>
> extension was correct :)
Fixed, thanks.
>> + .target = xt_snat_target_v1,
>> + .targetsize = sizeof(struct nf_nat_range),
>> + .table = "nat",
>> + .hooks = (1 << NF_INET_POST_ROUTING) |
>> + (1 << NF_INET_LOCAL_OUT),
>> + .me = THIS_MODULE,
>
> .family = NFPROTO_UNSPEC,
>
> Just for completeness.
This is obvious.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists