| 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
| ||
|
Date: Tue, 12 Oct 2010 19:30:30 +0300
From: "Rémi Denis-Courmont" <remi@...lab.net>
To: Kumar A Sanghvi <kumar.sanghvi@...ricsson.com>
Cc: remi.denis-courmont@...ia.com, davem@...emloft.net,
netdev@...r.kernel.org, linus.walleij@...ricsson.com,
gulshan.karmani@...ricsson.com, sudeep.divakaran@...ricsson.com
Subject: Re: [PATCH] Phonet: 'connect' socket implementation for Pipe controller
Hello,
Just a few comments...
Le mardi 12 octobre 2010 18:26:51 Kumar A Sanghvi, vous avez écrit :
> diff --git a/include/net/phonet/pep.h b/include/net/phonet/pep.h
> index def6cfa..b60b28c 100644
> --- a/include/net/phonet/pep.h
> +++ b/include/net/phonet/pep.h
> @@ -802,6 +682,42 @@ static void pipe_destruct(struct sock *sk)
> skb_queue_purge(&pn->ctrlreq_queue);
> }
>
> +#ifdef CONFIG_PHONET_PIPECTRLR
> +static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
> +{
> + struct pep_sock *pn = pep_sk(sk);
> + static u8 host_pref_rx_fc[3] = {3, 2, 1}, host_req_tx_fc[3] = {3, 2, 1};
Why is this 'static' ? Doesn't that break concurrent uses?
> + u8 remote_pref_rx_fc[3], remote_req_tx_fc[3];
> + u8 negotiated_rx_fc, negotiated_tx_fc;
> + int ret;
> +
> + pipe_get_flow_info(sk, skb, remote_pref_rx_fc,
> + remote_req_tx_fc);
> + negotiated_tx_fc = pipe_negotiate_fc(remote_req_tx_fc,
> + host_pref_rx_fc,
> + sizeof(host_pref_rx_fc));
> + negotiated_rx_fc = pipe_negotiate_fc(host_req_tx_fc,
> + remote_pref_rx_fc,
> + sizeof(host_pref_rx_fc));
> +
> + pn->pipe_state = PIPE_DISABLED;
> + sk->sk_state = TCP_SYN_RECV;
> + sk->sk_backlog_rcv = pipe_do_rcv;
> + sk->sk_destruct = pipe_destruct;
> + pn->rx_credits = 0;
> + pn->rx_fc = negotiated_rx_fc;
> + pn->tx_fc = negotiated_tx_fc;
> + sk->sk_state_change(sk);
> +
> + ret = pipe_handler_send_created_ind(sk,
> + PNS_PIPE_CREATED_IND_UTID,
> + PNS_PIPE_CREATED_IND
> + );
> +
> + return ret;
> +}
> +#endif
> +
> static int pep_connreq_rcv(struct sock *sk, struct sk_buff *skb)
> {
> struct sock *newsk;
> diff --git a/net/phonet/socket.c b/net/phonet/socket.c
> index aca8fba..123a374 100644
> --- a/net/phonet/socket.c
> +++ b/net/phonet/socket.c
> @@ -225,6 +225,102 @@ static int pn_socket_autobind(struct socket *sock)
> return 0; /* socket was already bound */
> }
>
> +static int pn_socket_connect(struct socket *sock, struct sockaddr *addr,
> + int len, int flags)
> +{
> + struct sock *sk = sock->sk;
> + struct sockaddr_pn *spn = (struct sockaddr_pn *)addr;
> + long timeo;
> + int err;
> +
> + lock_sock(sk);
> +
> + if (len < sizeof(struct sockaddr_pn))
> + return -EINVAL;
> + if (spn->spn_family != AF_PHONET)
> + return -EAFNOSUPPORT;
You should move lock_sock(sk); here, I think.
> +
> + switch (sock->state) {
> + case SS_UNCONNECTED:
> + sk->sk_state = TCP_CLOSE;
> + break;
> + case SS_CONNECTING:
> + switch (sk->sk_state) {
> + case TCP_SYN_RECV:
> + sock->state = SS_CONNECTED;
> + err = -EISCONN;
> + goto out;
> + case TCP_CLOSE:
> + err = -EALREADY;
> + if (flags & O_NONBLOCK)
> + goto out;
> + goto wait_connect;
> + break;
I think the kernel policy is against redumdant break statements.
> + }
> + break;
> + case SS_CONNECTED:
> + switch (sk->sk_state) {
> + case TCP_SYN_RECV:
> + err = -EISCONN;
> + goto out;
> + case TCP_CLOSE:
> + sock->state = SS_UNCONNECTED;
> + break;
> + }
> + break;
> + case SS_DISCONNECTING:
> + case SS_FREE:
> + break;
> + }
> + sk->sk_state = TCP_CLOSE;
> + sock->state = SS_UNCONNECTED;
This is dead code...
> + sk_stream_kill_queues(sk);
> +
> +
> + sock->state = SS_CONNECTING;
...because of this ^ .
> + err = sk->sk_prot->connect(sk, addr, len);
> + if (err < 0) {
> + sock->state = SS_UNCONNECTED;
> + sk->sk_state = TCP_CLOSE;
> + goto out;
> + }
> +
> + err = -EINPROGRESS;
> +wait_connect:
> + if (sk->sk_state != TCP_SYN_RECV && (flags & O_NONBLOCK))
> + goto out;
> +
> + timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> + release_sock(sk);
> +
> + err = -ERESTARTSYS;
> + timeo = wait_event_interruptible_timeout(*sk_sleep(sk),
> + sk->sk_state != TCP_CLOSE,
> + timeo);
> +
> + lock_sock(sk);
> + if (timeo < 0)
> + goto out; /* -ERESTARTSYS */
> +
> + err = -ETIMEDOUT;
> + if (timeo == 0 && sk->sk_state != TCP_SYN_RECV)
> + goto out;
> +
> + if (sk->sk_state != TCP_SYN_RECV) {
> + sock->state = SS_UNCONNECTED;
> + err = sock_error(sk);
> + if (!err)
> + err = -ECONNREFUSED;
> + goto out;
> + }
> + sock->state = SS_CONNECTED;
> + err = 0;
> +
> +out:
> + release_sock(sk);
> + return err;
> +}
> +
> static int pn_socket_accept(struct socket *sock, struct socket *newsock,
> int flags)
> {
--
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis
--
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