Partha,
I think it would be simpler to just always keep the packet list on the
stack, like it was before I introduced the faulty patch.
Yes, it costs a couple of cycles extra, but simplicity is also an
important factor here.
///jon
On 02/24/2016 10:23 AM, Parthasarathy Bhuvaragan wrote:
> In Commit 94153e36e709e ("tipc: use existing sk_write_queue for
> outgoing packet chain"), we assume that we fill & empty the socket's
> sk_write_queue within the same lock_sock() session.
>
> This is not true if the link is congested. During congestion, the
> socket lock is released while we wait for the congestion to cease.
> This implementation causes a nullptr exception, if the user space
> program has several threads accessing the same socket descriptor.
>
> Consider two threads of the same program performing the following:
> Thread1 Thread2
> -------------------- ----------------------
> Enter tipc_sendmsg() Enter tipc_sendmsg()
> lock_sock() lock_sock()
> Enter tipc_link_xmit(), ret=ELINKCONG spin on socket lock..
> sk_wait_event() :
> release_sock() grab socket lock
> : Enter tipc_link_xmit(), ret=0
> : release_sock()
> Wakeup after congestion
> lock_sock()
> skb = skb_peek(pktchain);
> !! TIPC_SKB_CB(skb)->wakeup_pending = tsk->link_cong;
>
> In this case, the second thread transmits the buffers belonging to both
> thread1 and thread2 successfully. When the first thread wakeup after the
> congestion it assumes that the pktchain is intact and operates on the skb's
> in it, which leads to nullptr exception as follows:
>
> [2102.439969] BUG: unable to handle kernel NULL pointer dereference at
> 00000000000000d0
> [2102.440074] IP: [<ffffffffa005f330>] __tipc_link_xmit+0x2b0/0x4d0 [tipc]
> [2102.440074] PGD 3fa3f067 PUD 3fa6b067 PMD 0
> [2102.440074] Oops: 0000 [#1] SMP
> [2102.440074] CPU: 2 PID: 244 Comm: sender Not tainted 3.12.28 #1
> [2102.440074] RIP: 0010:[<ffffffffa005f330>] [<ffffffffa005f330>]
> __tipc_link_xmit+0x2b0/0x4d0 [tipc]
> [...]
> [2102.440074] Call Trace:
> [2102.440074] [<ffffffff8163f0b9>] ? schedule+0x29/0x70
> [2102.440074] [<ffffffffa006a756>] ? tipc_node_unlock+0x46/0x170 [tipc]
> [2102.440074] [<ffffffffa005f761>] tipc_link_xmit+0x51/0xf0 [tipc]
> [2102.440074] [<ffffffffa006d8ae>] tipc_send_stream+0x11e/0x4f0 [tipc]
> [2102.440074] [<ffffffff8106b150>] ? __wake_up_sync+0x20/0x20
> [2102.440074] [<ffffffffa006dc9c>] tipc_send_packet+0x1c/0x20 [tipc]
> [2102.440074] [<ffffffff81502478>] sock_sendmsg+0xa8/0xd0
> [2102.440074] [<ffffffff81507895>] ? release_sock+0x145/0x170
> [2102.440074] [<ffffffff815030d8>] ___sys_sendmsg+0x3d8/0x3e0
> [2102.440074] [<ffffffff816426ae>] ? _raw_spin_unlock+0xe/0x10
> [2102.440074] [<ffffffff81115c2a>] ? handle_mm_fault+0x6ca/0x9d0
> [2102.440074] [<ffffffff8107dd65>] ? set_next_entity+0x85/0xa0
> [2102.440074] [<ffffffff816426de>] ? _raw_spin_unlock_irq+0xe/0x20
> [2102.440074] [<ffffffff8107463c>] ? finish_task_switch+0x5c/0xc0
> [2102.440074] [<ffffffff8163ea8c>] ? __schedule+0x34c/0x950
> [2102.440074] [<ffffffff81504e12>] __sys_sendmsg+0x42/0x80
> [2102.440074] [<ffffffff81504e62>] SyS_sendmsg+0x12/0x20
> [2102.440074] [<ffffffff8164aed2>] system_call_fastpath+0x16/0x1b
>
> In this commit, we maintain the buffer list in the stack during congestion
> and use sk_write_queue otherwise.
>
> Fixes:
> 94153e36e709e ("tipc: use existing sk_write_queue for outgoing packet chain")
> Signed-off-by: Parthasarathy Bhuvaragan
> <[email protected]>
> ---
> net/tipc/socket.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 69c29050f14a..bb5875c18c98 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -674,6 +674,7 @@ static int tipc_sendmcast(struct socket *sock, struct
> tipc_name_seq *seq,
> struct net *net = sock_net(sk);
> struct tipc_msg *mhdr = &tsk->phdr;
> struct sk_buff_head *pktchain = &sk->sk_write_queue;
> + struct sk_buff_head tmpq;
> struct iov_iter save = msg->msg_iter;
> uint mtu;
> int rc;
> @@ -687,6 +688,11 @@ static int tipc_sendmcast(struct socket *sock, struct
> tipc_name_seq *seq,
> msg_set_nameupper(mhdr, seq->upper);
> msg_set_hdr_sz(mhdr, MCAST_H_SIZE);
>
> + if (likely(!skb_queue_empty(pktchain))) {
> + skb_queue_head_init(&tmpq);
> + pktchain = &tmpq;
> + }
> +
> new_mtu:
> mtu = tipc_bcast_get_mtu(net);
> rc = tipc_msg_build(mhdr, msg, 0, dsz, mtu, pktchain);
> @@ -864,6 +870,7 @@ static int __tipc_sendmsg(struct socket *sock, struct
> msghdr *m, size_t dsz)
> struct tipc_msg *mhdr = &tsk->phdr;
> u32 dnode, dport;
> struct sk_buff_head *pktchain = &sk->sk_write_queue;
> + struct sk_buff_head tmpq;
> struct sk_buff *skb;
> struct tipc_name_seq *seq;
> struct iov_iter save;
> @@ -924,6 +931,11 @@ static int __tipc_sendmsg(struct socket *sock, struct
> msghdr *m, size_t dsz)
> msg_set_hdr_sz(mhdr, BASIC_H_SIZE);
> }
>
> + if (likely(!skb_queue_empty(pktchain))) {
> + skb_queue_head_init(&tmpq);
> + pktchain = &tmpq;
> + }
> +
> save = m->msg_iter;
> new_mtu:
> mtu = tipc_node_get_mtu(net, dnode, tsk->portid);
> @@ -1017,6 +1029,7 @@ static int __tipc_send_stream(struct socket *sock,
> struct msghdr *m, size_t dsz)
> struct tipc_sock *tsk = tipc_sk(sk);
> struct tipc_msg *mhdr = &tsk->phdr;
> struct sk_buff_head *pktchain = &sk->sk_write_queue;
> + struct sk_buff_head tmpq;
> DECLARE_SOCKADDR(struct sockaddr_tipc *, dest, m->msg_name);
> u32 portid = tsk->portid;
> int rc = -EINVAL;
> @@ -1045,6 +1058,11 @@ static int __tipc_send_stream(struct socket *sock,
> struct msghdr *m, size_t dsz)
> timeo = sock_sndtimeo(sk, m->msg_flags & MSG_DONTWAIT);
> dnode = tsk_peer_node(tsk);
>
> + if (likely(!skb_queue_empty(pktchain))) {
> + skb_queue_head_init(&tmpq);
> + pktchain = &tmpq;
> + }
> +
> next:
> save = m->msg_iter;
> mtu = tsk->max_pkt;
> @@ -1052,6 +1070,7 @@ next:
> rc = tipc_msg_build(mhdr, m, sent, send, mtu, pktchain);
> if (unlikely(rc < 0))
> return rc;
> +
> do {
> if (likely(!tsk_conn_cong(tsk))) {
> rc = tipc_node_xmit(net, pktchain, dnode, portid);
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
tipc-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
