| Message ID | 1346629684.2563.78.camel@edumazet-glaptop |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet <eric.dumazet@gmail.com> wrote: > Sami Farin reported crashes in xt_LOG because it assumes skb->sk is a > full blown socket. > > But with TCP early demux, we can have skb->sk pointing to a timewait > socket. > > Same fix is needed in netfnetlink_log Looks good, but IMHO it is very un-intuitive that skb->sk might be a pointer to an object that is not struct sock (or a compatible object). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-09-03 at 01:48 +0200, Eric Dumazet wrote: > Sami Farin reported crashes in xt_LOG because it assumes skb->sk is a > full blown socket. > > But with TCP early demux, we can have skb->sk pointing to a timewait > socket. > > Same fix is needed in netfnetlink_log > > Diagnosed-by: Florian Westphal <fw@strlen.de> > Reported-by: Sami Farin <hvtaifwkbgefbaei@gmail.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/netfilter/nfnetlink_log.c | 14 +++++++------ > net/netfilter/xt_LOG.c | 33 ++++++++++++++++---------------- > 2 files changed, 25 insertions(+), 22 deletions(-) > > diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c > index 14e2f39..5cfb5be 100644 > --- a/net/netfilter/nfnetlink_log.c > +++ b/net/netfilter/nfnetlink_log.c > @@ -381,6 +381,7 @@ __build_packet_message(struct nfulnl_instance *inst, > struct nlmsghdr *nlh; > struct nfgenmsg *nfmsg; > sk_buff_data_t old_tail = inst->skb->tail; > + struct sock *sk; > > nlh = nlmsg_put(inst->skb, 0, 0, > NFNL_SUBSYS_ULOG << 8 | NFULNL_MSG_PACKET, > @@ -499,18 +500,19 @@ __build_packet_message(struct nfulnl_instance *inst, > } > > /* UID */ > - if (skb->sk) { > - read_lock_bh(&skb->sk->sk_callback_lock); > - if (skb->sk->sk_socket && skb->sk->sk_socket->file) { > - struct file *file = skb->sk->sk_socket->file; > + sk = skb->sk; > + if (sk && sk->sk_state != TCP_TIME_WAIT) { > + read_lock_bh(&sk->sk_callback_lock); > + if (sk->sk_socket && sk->sk_socket->file) { > + struct file *file = sk->sk_socket->file; > __be32 uid = htonl(file->f_cred->fsuid); > __be32 gid = htonl(file->f_cred->fsgid); > - read_unlock_bh(&skb->sk->sk_callback_lock); > + read_unlock_bh(&sk->sk_callback_lock); > if (nla_put_be32(inst->skb, NFULA_UID, uid) || > nla_put_be32(inst->skb, NFULA_GID, gid)) > goto nla_put_failure; > } else > - read_unlock_bh(&skb->sk->sk_callback_lock); > + read_unlock_bh(&sk->sk_callback_lock); > } > > /* local sequence number */ > diff --git a/net/netfilter/xt_LOG.c b/net/netfilter/xt_LOG.c > index ff5f75f..2a4f969 100644 > --- a/net/netfilter/xt_LOG.c > +++ b/net/netfilter/xt_LOG.c > @@ -145,6 +145,19 @@ static int dump_tcp_header(struct sbuff *m, const struct sk_buff *skb, > return 0; > } > > +static void dump_sk_uid_gid(struct sbuff *m, struct sock *sk) > +{ > + if (!sk || sk->sk_state == TCP_TIME_WAIT) > + return; > + > + read_lock_bh(&sk->sk_callback_lock); > + if (sk->sk_socket && sk->sk_socket->file) > + sb_add(m, "UID=%u GID=%u ", > + sk->sk_socket->file->f_cred->fsuid, > + sk->sk_socket->file->f_cred->fsgid); > + read_unlock_bh(&sk->sk_callback_lock); > +} > + > /* One level of recursion won't kill us */ > static void dump_ipv4_packet(struct sbuff *m, > const struct nf_loginfo *info, > @@ -361,14 +374,8 @@ static void dump_ipv4_packet(struct sbuff *m, > } > > /* Max length: 15 "UID=4294967295 " */ > - if ((logflags & XT_LOG_UID) && !iphoff && skb->sk) { > - read_lock_bh(&skb->sk->sk_callback_lock); > - if (skb->sk->sk_socket && skb->sk->sk_socket->file) > - sb_add(m, "UID=%u GID=%u ", > - skb->sk->sk_socket->file->f_cred->fsuid, > - skb->sk->sk_socket->file->f_cred->fsgid); > - read_unlock_bh(&skb->sk->sk_callback_lock); > - } > + if ((logflags & XT_LOG_UID) && !iphoff) > + dump_sk_uid_gid(m, skb->sk); > > /* Max length: 16 "MARK=0xFFFFFFFF " */ > if (!iphoff && skb->mark) > @@ -717,14 +724,8 @@ static void dump_ipv6_packet(struct sbuff *m, > } > > /* Max length: 15 "UID=4294967295 " */ > - if ((logflags & XT_LOG_UID) && recurse && skb->sk) { > - read_lock_bh(&skb->sk->sk_callback_lock); > - if (skb->sk->sk_socket && skb->sk->sk_socket->file) > - sb_add(m, "UID=%u GID=%u ", > - skb->sk->sk_socket->file->f_cred->fsuid, > - skb->sk->sk_socket->file->f_cred->fsgid); > - read_unlock_bh(&skb->sk->sk_callback_lock); > - } > + if ((logflags & XT_LOG_UID) && recurse) > + dump_sk_uid_gid(m, skb->sk); > > /* Max length: 16 "MARK=0xFFFFFFFF " */ > if (!recurse && skb->mark) > Hi David This patch was marked "Not applicable" in Patchwork ( http://patchwork.ozlabs.org/patch/181275/ ) It seems a mistake, since it should be applied ? Tell me if I should resend it Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 04 Sep 2012 09:55:25 +0200 > This patch was marked "Not applicable" in Patchwork > ( http://patchwork.ozlabs.org/patch/181275/ ) > > It seems a mistake, since it should be applied ? Not applicable can mean "doesn't go through my tree" This is netfilter stuff, so please submit it via the netfilter folks :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 03, 2012 at 01:48:04 +0200, Eric Dumazet wrote: > Sami Farin reported crashes in xt_LOG because it assumes skb->sk is a > full blown socket. > > But with TCP early demux, we can have skb->sk pointing to a timewait > socket. > > Same fix is needed in netfnetlink_log with ip_early_demux=0 and without these patches I did not get panics for 7 days, and with the patches and ip_early_demux=1 it has worked OK for 5 days, thanks.
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index 14e2f39..5cfb5be 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -381,6 +381,7 @@ __build_packet_message(struct nfulnl_instance *inst, struct nlmsghdr *nlh; struct nfgenmsg *nfmsg; sk_buff_data_t old_tail = inst->skb->tail; + struct sock *sk; nlh = nlmsg_put(inst->skb, 0, 0, NFNL_SUBSYS_ULOG << 8 | NFULNL_MSG_PACKET, @@ -499,18 +500,19 @@ __build_packet_message(struct nfulnl_instance *inst, } /* UID */ - if (skb->sk) { - read_lock_bh(&skb->sk->sk_callback_lock); - if (skb->sk->sk_socket && skb->sk->sk_socket->file) { - struct file *file = skb->sk->sk_socket->file; + sk = skb->sk; + if (sk && sk->sk_state != TCP_TIME_WAIT) { + read_lock_bh(&sk->sk_callback_lock); + if (sk->sk_socket && sk->sk_socket->file) { + struct file *file = sk->sk_socket->file; __be32 uid = htonl(file->f_cred->fsuid); __be32 gid = htonl(file->f_cred->fsgid); - read_unlock_bh(&skb->sk->sk_callback_lock); + read_unlock_bh(&sk->sk_callback_lock); if (nla_put_be32(inst->skb, NFULA_UID, uid) || nla_put_be32(inst->skb, NFULA_GID, gid)) goto nla_put_failure; } else - read_unlock_bh(&skb->sk->sk_callback_lock); + read_unlock_bh(&sk->sk_callback_lock); } /* local sequence number */ diff --git a/net/netfilter/xt_LOG.c b/net/netfilter/xt_LOG.c index ff5f75f..2a4f969 100644 --- a/net/netfilter/xt_LOG.c +++ b/net/netfilter/xt_LOG.c @@ -145,6 +145,19 @@ static int dump_tcp_header(struct sbuff *m, const struct sk_buff *skb, return 0; } +static void dump_sk_uid_gid(struct sbuff *m, struct sock *sk) +{ + if (!sk || sk->sk_state == TCP_TIME_WAIT) + return; + + read_lock_bh(&sk->sk_callback_lock); + if (sk->sk_socket && sk->sk_socket->file) + sb_add(m, "UID=%u GID=%u ", + sk->sk_socket->file->f_cred->fsuid, + sk->sk_socket->file->f_cred->fsgid); + read_unlock_bh(&sk->sk_callback_lock); +} + /* One level of recursion won't kill us */ static void dump_ipv4_packet(struct sbuff *m, const struct nf_loginfo *info, @@ -361,14 +374,8 @@ static void dump_ipv4_packet(struct sbuff *m, } /* Max length: 15 "UID=4294967295 " */ - if ((logflags & XT_LOG_UID) && !iphoff && skb->sk) { - read_lock_bh(&skb->sk->sk_callback_lock); - if (skb->sk->sk_socket && skb->sk->sk_socket->file) - sb_add(m, "UID=%u GID=%u ", - skb->sk->sk_socket->file->f_cred->fsuid, - skb->sk->sk_socket->file->f_cred->fsgid); - read_unlock_bh(&skb->sk->sk_callback_lock); - } + if ((logflags & XT_LOG_UID) && !iphoff) + dump_sk_uid_gid(m, skb->sk); /* Max length: 16 "MARK=0xFFFFFFFF " */ if (!iphoff && skb->mark) @@ -717,14 +724,8 @@ static void dump_ipv6_packet(struct sbuff *m, } /* Max length: 15 "UID=4294967295 " */ - if ((logflags & XT_LOG_UID) && recurse && skb->sk) { - read_lock_bh(&skb->sk->sk_callback_lock); - if (skb->sk->sk_socket && skb->sk->sk_socket->file) - sb_add(m, "UID=%u GID=%u ", - skb->sk->sk_socket->file->f_cred->fsuid, - skb->sk->sk_socket->file->f_cred->fsgid); - read_unlock_bh(&skb->sk->sk_callback_lock); - } + if ((logflags & XT_LOG_UID) && recurse) + dump_sk_uid_gid(m, skb->sk); /* Max length: 16 "MARK=0xFFFFFFFF " */ if (!recurse && skb->mark)
Sami Farin reported crashes in xt_LOG because it assumes skb->sk is a full blown socket. But with TCP early demux, we can have skb->sk pointing to a timewait socket. Same fix is needed in netfnetlink_log Diagnosed-by: Florian Westphal <fw@strlen.de> Reported-by: Sami Farin <hvtaifwkbgefbaei@gmail.com> Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/netfilter/nfnetlink_log.c | 14 +++++++------ net/netfilter/xt_LOG.c | 33 ++++++++++++++++---------------- 2 files changed, 25 insertions(+), 22 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html