| 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
| ||
|
Message-Id: <201009290125.07068.remi.denis-courmont@nokia.com>
Date: Wed, 29 Sep 2010 01:25:06 +0300
From: "Rémi Denis-Courmont"
<remi.denis-courmont@...ia.com>
To: ext Kumar A Sanghvi <kumar.sanghvi@...ricsson.com>
Cc: "davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"STEricsson_nomadik_linux@...t.st.com"
<STEricsson_nomadik_linux@...t.st.com>,
"sudeep.divakaran@...ricsson.com" <sudeep.divakaran@...ricsson.com>,
"gulshan.karmani@...ricsson.com" <gulshan.karmani@...ricsson.com>,
Linus Walleij <linus.walleij@...ricsson.com>
Subject: Re: [PATCH v2 1/2] Phonet: Implement Pipe Controller to support Nokia Slim Modems
On Monday 27 September 2010 08:07:59 ext Kumar A Sanghvi, you wrote:
> From: Kumar Sanghvi <kumar.sanghvi@...ricsson.com>
>
> Phonet stack assumes the presence of Pipe Controller, either in Modem or
> on Application Processing Engine user-space for the Pipe data.
> Nokia Slim Modems like WG2.5 used in ST-Ericsson U8500 platform do not
> implement Pipe controller in them.
> This patch adds Pipe Controller implemenation to Phonet stack to support
> Pipe data over Phonet stack for Nokia Slim Modems.
>
> Signed-off-by: Kumar Sanghvi <kumar.sanghvi@...ricsson.com>
> Acked-by: Linus Walleij <linus.walleij@...ricsson.com>
> ---
> Changes:
>
> -v2: Correction for header retrieving after pskb_may_pull
>
> include/linux/phonet.h | 5 +
> include/net/phonet/pep.h | 21 +++
> net/phonet/Kconfig | 11 ++
> net/phonet/pep.c | 448
> +++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 479
> insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/phonet.h b/include/linux/phonet.h
> index 85e14a8..96f5625 100644
> --- a/include/linux/phonet.h
> +++ b/include/linux/phonet.h
> @@ -36,6 +36,11 @@
> /* Socket options for SOL_PNPIPE level */
> #define PNPIPE_ENCAP 1
> #define PNPIPE_IFINDEX 2
> +#define PNPIPE_CREATE 3
> +#define PNPIPE_ENABLE 4
> +#define PNPIPE_DISABLE 5
> +#define PNPIPE_DESTROY 6
> +#define PNPIPE_INQ 7
As far as I know, you don't need to do that in kernel space. I don't know the
internals of the STE modem. Regardless of having or not having a pipe
controller, Linux userspace can send pipe messages using a plain Phonet
datagram socket. This avoids adding ioctl()'s and protocol stuff in kernel
space. Then, as far as kernel is concerned, only small changes to the data
path would be required.
Both the Nokia modem plugin for oFono, and the (closed-source) Nokia N900 CSD-
GPRS service work that way. In other words, the pipe 'signaling' is done in
userspace, while the pipe 'data' is done in kernel space for optimal
performance.
For some background - Phonet pipes work very much like FTP. There are two
endpoints exchaning data, and one client ('owner') deciding which endpoints
and when to establish a pipe between. In most case the client is also one of
the endpoint, but this is not required. With this patch, the client is tied to
being one of the endpoint. Arguably, this is not a problem for most usecases.
But I am not sure this belongs in *kernel* space. Sticking to the same
comparison: with FTP, you have TCP (data path) in kernel, but not FTP itself
(signaling).
> @@ -791,6 +1171,48 @@ static int pep_setsockopt(struct sock *sk, int level,
> int optname,
>
> lock_sock(sk);
> switch (optname) {
> +#ifdef CONFIG_PHONET_PIPECTRLR
> + case PNPIPE_CREATE:
> + if (val) {
> + if (pn->pipe_state > PIPE_IDLE) {
> + err = -EFAULT;
Why EFAULT here? I can't see any user-space memory access failure.
> + break;
> + }
> + remote_pep = val & 0xFFFF;
> + pipe_handle = (val >> 16) & 0xFF;
> + pn->remote_pep = remote_pep;
> + err = pipe_handler_create_pipe(sk, pipe_handle,
> + PNPIPE_CREATE);
> + break;
> + }
> +
> + case PNPIPE_ENABLE:
> + if (pn->pipe_state != PIPE_DISABLED) {
> + err = -EFAULT;
Same here.
> + break;
> + }
> + err = pipe_handler_enable_pipe(sk, PNPIPE_ENABLE);
> + break;
> +
> + case PNPIPE_DISABLE:
> + if (pn->pipe_state != PIPE_ENABLED) {
> + err = -EFAULT;
Ditto.
> + break;
> + }
> +
> + err = pipe_handler_enable_pipe(sk, PNPIPE_DISABLE);
> + break;
> +
> + case PNPIPE_DESTROY:
> + if (pn->pipe_state < PIPE_DISABLED) {
> + err = -EFAULT;
And here,
> @@ -877,11 +1313,11 @@ static int pipe_skb_send(struct sock *sk, struct
> sk_buff *skb) } else
> ph->message_id = PNS_PIPE_DATA;
> ph->pipe_handle = pn->pipe_handle;
> -
> - err = pn_skb_send(sk, skb, &pipe_srv);
> - if (err && pn_flow_safe(pn->tx_fc))
> - atomic_inc(&pn->tx_credits);
> - return err;
> +#ifdef CONFIG_PHONET_PIPECTRLR
> + return pn_skb_send(sk, skb, &spn);
> +#else
> + return pn_skb_send(sk, skb, &pipe_srv);
> +#endif
> }
This reintroduces the bug that I fixed in
1a98214feef2221cd7c24b17cd688a5a9d85b2ea :-(
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
--
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