netdev - Re: [PATCH 3/6] Phonet: Pipe End Point for Phonet Pipes protocol

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081002134137.GA17843@ghostprotocols.net>
Date:	Thu, 2 Oct 2008 10:41:37 -0300
From:	Arnaldo Carvalho de Melo <acme@...hat.com>
To:	Rémi Denis-Courmont 
	<remi.denis-courmont@...ia.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 3/6] Phonet: Pipe End Point for Phonet Pipes protocol

Em Thu, Oct 02, 2008 at 01:50:52PM +0300, Rémi Denis-Courmont escreveu:
> On Wednesday 01 October 2008 16:18:56 ext Arnaldo Carvalho de Melo, you wrote:
> > > +static struct sock *pep_find_pipe(const struct hlist_head *hlist,
> > > +					const struct sockaddr_pn *dst,
> > > +					u8 pipe_handle)
> > > +{
> > > +	struct hlist_node *node;
> > > +	struct sock *sknode;
> > > +	u16 dobj = pn_sockaddr_get_object(dst);
> >
> > What is the lock that protects this list traversal?
> 
> Either (accepted or unaccepted) sock lists should only be used with the sock 
> lock of the listening sock. Is that insufficient?
> 
> > > +static int pep_wait_connreq(struct sock *sk, int noblock)
> >
> > This function looks familiar... inet_csk_accept,
> > inet_csk_wait_for_connect...
> 
> I don't recall why I gave up on using request_sock and listen_sock there.
> 
> > perhaps we need a connection_sock father 
> > for inet_connection_sock? :-)
> 
> But you cannot have double inheritance, right? inet_sock and 
> connection_sock... I guess that's why listen_sock is _not_ a sock.

I'm not saying that you could use in its current form, only that it
looks as something to think about after you get phonet merged and
polished.
 
> > > +{
> > > +	struct task_struct *tsk = current;
> > > +	struct pep_sock *pn = pep_sk(sk);
> > > +	long timeo = sock_rcvtimeo(sk, noblock);
> > > +
> > > +	for (;;) {
> > > +		DEFINE_WAIT(wait);
> > > +
> > > +		if (sk->sk_state != TCP_LISTEN)
> > > +			return -EINVAL;
> > > +		if (!hlist_empty(&pn->ackq))
> > > +			break;
> > > +		if (!timeo)
> > > +			return -EWOULDBLOCK;
> > > +		if (signal_pending(tsk))
> > > +			return sock_intr_errno(timeo);
> > > +
> > > +		prepare_to_wait_exclusive(&sk->sk_socket->wait, &wait,
> > > +						TASK_INTERRUPTIBLE);
> > > +		release_sock(sk);
> > > +		timeo = schedule_timeout(timeo);
> > > +		lock_sock(sk);
> > > +		finish_wait(&sk->sk_socket->wait, &wait);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct sock *pep_sock_accept(struct sock *sk, int flags, int
> > > *errp) +{
> > > +	struct pep_sock *pn = pep_sk(sk);
> > > +	struct sock *newsk = NULL;
> > > +	struct sk_buff *oskb;
> > > +	int err;
> > > +
> > > +	lock_sock(sk);
> > > +	err = pep_wait_connreq(sk, flags & O_NONBLOCK);
> > > +	if (err)
> > > +		goto out;
> > > +
> > > +	newsk = __sk_head(&pn->ackq);
> > > +
> > > +	oskb = skb_dequeue(&newsk->sk_receive_queue);
> > > +	err = pep_accept_conn(newsk, oskb);
> > > +	if (err) {
> > > +		skb_queue_head(&newsk->sk_receive_queue, oskb);
> > > +		newsk = NULL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	sock_hold(sk);
> > > +	pep_sk(newsk)->listener = sk;
> > > +
> > > +	sock_hold(newsk);
> > > +	sk_del_node_init(newsk);
> > > +	sk_acceptq_removed(sk);
> > > +	sk_add_node(newsk, &pn->hlist);
> > > +	__sock_put(newsk);
> > > +
> > > +out:
> > > +	release_sock(sk);
> > > +	*errp = err;
> > > +	return newsk;
> > > +}
> > > +
> > > +static int pep_ioctl(struct sock *sk, int cmd, unsigned long arg)
> > > +{
> > > +	int answ;
> > > +
> > > +	switch (cmd) {
> > > +	case SIOCINQ:
> > > +		if (sk->sk_state == TCP_LISTEN)
> > > +			return -EINVAL;
> > > +
> > > +		lock_sock(sk);
> > > +		if (!skb_queue_empty(&sk->sk_receive_queue))
> > > +			answ = skb_peek(&sk->sk_receive_queue)->len;
> > > +		else
> > > +			answ = 0;
> > > +		release_sock(sk);
> > > +		return put_user(answ, (int __user *)arg);
> >
> > this is so common I wonder we if a helper wouldn't help 8) Look at
> > dccp_ioctl before Ilpo does 8)
> 
> It is not so common with the next patch which checks for urgent inline... As 
> far as I know, there is no common queue for out-of-band data.

I saw that, but out-of-band data is also something up for study wheter
introductin common infrastructure is feasible.

- Arnaldo
--
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