Message ID | 1346626385.2563.44.camel@edumazet-glaptop |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, 2012-09-03 at 00:53 +0200, Eric Dumazet wrote: > First I thought changing demux to not do the lookup of TIMEWAIT slot in > __inet_lookup_established(), then it sounds not optimal to redo the full > lookup for ESTABLISHED sockets later in TCP stack. > > So it seems we should fix xt_LOG instead > > Thanks Florian ! > > [PATCH] xt_LOG: take care of timewait sockets > > 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. Seems other fixes are needed, for example in nfnetlink_log.c I'll send a v2 -- 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 Monday 2012-09-03 00:53, Eric Dumazet wrote: >[PATCH] xt_LOG: take care of timewait sockets > >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. > >+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); xt_owner.c is also using f_cred, so it might need the same, does it not? -- 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-10 at 07:02 +0200, Jan Engelhardt wrote: > On Monday 2012-09-03 00:53, Eric Dumazet wrote: > >[PATCH] xt_LOG: take care of timewait sockets > > > >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. > > > >+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); > > xt_owner.c is also using f_cred, so it might need the same, > does it not? Right. AFAIK, xt_owner would make little sense in input path, no ? static struct xt_match owner_mt_reg __read_mostly = { .name = "owner", .revision = 1, .family = NFPROTO_UNSPEC, .checkentry = owner_check, .match = owner_mt, .matchsize = sizeof(struct xt_owner_match_info), .hooks = (1 << NF_INET_LOCAL_OUT) | (1 << NF_INET_POST_ROUTING), .me = THIS_MODULE, }; So it seems we have nothing to do at this moment. -- 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
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)