From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia•com>
To: ext Hemant Vilas RAMDASI <hemant.ramdasi@stericsson•com>,
netdev@vger•kernel.org
Subject: Re: [PATCH] Phonet: set the pipe handle using setsockopt
Date: Mon, 14 Nov 2011 11:29:58 +0200 [thread overview]
Message-ID: <2168215.tYVKKNoJjY@hector> (raw)
In-Reply-To: <1321257210-17200-1-git-send-email-hemant.ramdasi@stericsson.com>
Le Lundi 14 Novembre 2011 13:23:30 ext Hemant Vilas RAMDASI a écrit :
> From: Dinesh Kumar Sharma <dinesh.sharma@stericsson•com>
>
> This provides flexibility to set the pipe handle
> using setsockopt. The pipe can be enabled (if disabled) later
> using ioctl.
>
> Signed-off-by: Hemant Ramdasi <hemant.ramdasi@stericsson•com>
> Signed-off-by: Dinesh Kumar Sharma <dinesh.sharma@stericsson•com>
> ---
> include/linux/phonet.h | 3 +
> net/phonet/pep.c | 105
> +++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 98
> insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/phonet.h b/include/linux/phonet.h
> index 6fb1384..4c00551 100644
> --- a/include/linux/phonet.h
> +++ b/include/linux/phonet.h
> @@ -37,6 +37,8 @@
> #define PNPIPE_ENCAP 1
> #define PNPIPE_IFINDEX 2
> #define PNPIPE_HANDLE 3
> +#define PNPIPE_ENABLE 4
> +#define PNPIPE_INITSTATE 5
>
> #define PNADDR_ANY 0
> #define PNADDR_BROADCAST 0xFC
> @@ -180,6 +182,7 @@ static inline __u8 pn_sockaddr_get_resource(const struct
> sockaddr_pn *spn) /* Phonet device ioctl requests */
> #ifdef __KERNEL__
> #define SIOCPNGAUTOCONF (SIOCDEVPRIVATE + 0)
> +#define SIOPNPIPE_ENABLE _IO(SIOCPNGAUTOCONF, 1)
Does this even work? I am not an expert on this, but I would think that
device-private controls are routed to the network device, not the socket. In
any case, it does not seem right.
>
> struct if_phonet_autoconf {
> uint8_t device;
> diff --git a/net/phonet/pep.c b/net/phonet/pep.c
> index f17fd84..48339b9 100644
> --- a/net/phonet/pep.c
> +++ b/net/phonet/pep.c
> @@ -533,6 +533,29 @@ static int pep_connresp_rcv(struct sock *sk, struct
> sk_buff *skb) return pipe_handler_send_created_ind(sk);
> }
>
> +static int pep_enableresp_rcv(struct sock *sk, struct sk_buff *skb)
> +{
> + struct pnpipehdr *hdr = pnp_hdr(skb);
> +
> + if (hdr->error_code != PN_PIPE_NO_ERROR)
> + return -ECONNREFUSED;
> +
> + return pep_indicate(sk, PNS_PIPE_ENABLED_IND, 0 /* sub-blocks */,
> + NULL, 0, GFP_ATOMIC);
> +
> +}
> +
> +static void pipe_start_flow_control(struct sock *sk)
> +{
> + struct pep_sock *pn = pep_sk(sk);
> +
> + if (!pn_flow_safe(pn->tx_fc)) {
> + atomic_set(&pn->tx_credits, 1);
> + sk->sk_write_space(sk);
> + }
> + pipe_grant_credits(sk, GFP_ATOMIC);
> +}
> +
> /* Queue an skb to an actively connected sock.
> * Socket lock must be held. */
> static int pipe_handler_do_rcv(struct sock *sk, struct sk_buff *skb)
> @@ -578,13 +601,25 @@ static int pipe_handler_do_rcv(struct sock *sk, struct
> sk_buff *skb) sk->sk_state = TCP_CLOSE_WAIT;
> break;
> }
> + if (pn->init_enable == PN_PIPE_DISABLE)
> + sk->sk_state = TCP_SYN_RECV;
> + else {
> + sk->sk_state = TCP_ESTABLISHED;
> + pipe_start_flow_control(sk);
> + }
> + break;
>
> - sk->sk_state = TCP_ESTABLISHED;
> - if (!pn_flow_safe(pn->tx_fc)) {
> - atomic_set(&pn->tx_credits, 1);
> - sk->sk_write_space(sk);
> + case PNS_PEP_ENABLE_RESP:
> + if (sk->sk_state != TCP_SYN_SENT)
> + break;
> +
> + if (pep_enableresp_rcv(sk, skb)) {
> + sk->sk_state = TCP_CLOSE_WAIT;
> + break;
> }
> - pipe_grant_credits(sk, GFP_ATOMIC);
> +
> + sk->sk_state = TCP_ESTABLISHED;
> + pipe_start_flow_control(sk);
> break;
>
> case PNS_PEP_DISCONNECT_RESP:
> @@ -863,14 +898,31 @@ static int pep_sock_connect(struct sock *sk, struct
> sockaddr *addr, int len) int err;
> u8 data[4] = { 0 /* sub-blocks */, PAD, PAD, PAD };
>
> - pn->pipe_handle = 1; /* anything but INVALID_HANDLE */
> + if (pn->pipe_handle == PN_PIPE_INVALID_HANDLE)
> + pn->pipe_handle = 1; /* anything but INVALID_HANDLE */
> +
> err = pipe_handler_request(sk, PNS_PEP_CONNECT_REQ,
> - PN_PIPE_ENABLE, data, 4);
> - if (err) {
> - pn->pipe_handle = PN_PIPE_INVALID_HANDLE;
The current backlog functions assume that pipe_handle = PN_PIPE_INVALID_HANDLE
if the socket is not yet connected. That's why the old code would clear the
pipe_handle always on error.
So it is not that simple.
> + pn->init_enable, data, 4);
> + if (err)
> return err;
> - }
> +
> + sk->sk_state = TCP_SYN_SENT;
> +
> + return 0;
> +}
> +
> +static int pep_sock_enable(struct sock *sk, struct sockaddr *addr, int len)
> +{
> + int err;
> +
> + err = pipe_handler_request(sk, PNS_PEP_ENABLE_REQ, PAD,
> + NULL, 0);
> +
> + if (err)
> + return err;
> +
> sk->sk_state = TCP_SYN_SENT;
> +
> return 0;
> }
>
> @@ -894,6 +946,16 @@ static int pep_ioctl(struct sock *sk, int cmd, unsigned
> long arg) answ = 0;
> release_sock(sk);
> return put_user(answ, (int __user *)arg);
> + break;
> +
> + case SIOPNPIPE_ENABLE:
> + if (sk->sk_state == TCP_SYN_SENT)
> + return -EBUSY;
> + else if (sk->sk_state == TCP_ESTABLISHED)
> + return -EISCONN;
> + else
> + return pep_sock_enable(sk, NULL, 0);
> + break;
> }
I strongly suspect insufficient locking here.
>
> return -ENOIOCTLCMD;
> @@ -959,6 +1021,18 @@ static int pep_setsockopt(struct sock *sk, int level,
> int optname, }
> goto out_norel;
>
> + case PNPIPE_HANDLE:
> + if ((sk->sk_state == TCP_CLOSE) &&
> + (val >= 0) && (val < PN_PIPE_INVALID_HANDLE))
> + pn->pipe_handle = val;
> + else
> + err = -EINVAL;
> + break;
Same problem regarding pipe_handle as above.
> +
> + case PNPIPE_INITSTATE:
> + pn->init_enable = !!val;
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> }
> @@ -994,6 +1068,17 @@ static int pep_getsockopt(struct sock *sk, int level,
> int optname, return -EINVAL;
> break;
>
> + case PNPIPE_ENABLE:
> + if (sk->sk_state == TCP_ESTABLISHED)
> + val = 1;
> + else
> + val = 0;
> + break;
Do you still need this read-only option?
> +
> + case PNPIPE_INITSTATE:
> + val = pn->init_enable;
> + break;
> +
> default:
> return -ENOPROTOOPT;
> }
--
Rémi Denis-Courmont
http://www.remlab.net/
next prev parent reply other threads:[~2011-11-14 9:29 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-14 7:53 [PATCH] Phonet: set the pipe handle using setsockopt Hemant Vilas RAMDASI
2011-11-14 9:29 ` Rémi Denis-Courmont [this message]
2011-11-14 10:36 ` Hemant-vilas RAMDASI
2011-11-16 6:30 ` Rémi Denis-Courmont
2011-11-16 8:46 ` Hemant-vilas RAMDASI
2011-11-16 10:15 ` Rémi Denis-Courmont
-- strict thread matches above, loose matches on Subject: below --
2011-11-09 11:20 Hemant Vilas RAMDASI
2011-11-09 15:00 ` Rémi Denis-Courmont
2011-11-10 9:44 ` Hemant-vilas RAMDASI
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2168215.tYVKKNoJjY@hector \
--to=remi.denis-courmont@nokia$(echo .)com \
--cc=hemant.ramdasi@stericsson$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox