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: Fri, 19 Sep 2008 09:33:33 +0300 From: "Rémi Denis-Courmont" <remi.denis-courmont@...ia.com> To: "ext Arnaldo Carvalho de Melo" <acme@...hat.com> Cc: netdev@...r.kernel.org Subject: Re: [PATCH 10/14] Phonet: Phonet datagram transport protocol On Tuesday 16 September 2008 20:06:44 ext Arnaldo Carvalho de Melo, you wrote: > > +/* > > + * Prepends an ISI header and sends a datagram. > > + */ > > +static int pn_send(struct sk_buff *skb, struct net_device *dev, > > + u16 dst, u16 src, u8 res) > > +{ > > + struct phonethdr *ph; > > + int err; > > + > > + if (skb->len > 0xfffd) { > > wow, what a magic number! :-) Taking into account that we push sizeof(*ph) in-between, this protects against protocol overflow a few lines down: ph->length = __cpu_to_be16(skb->len + 2 - sizeof(*ph)); I guess it deserves a /* comment */. By the way, is the protocol stack or the device driver responsible for not transmitting above the MTU? > > + skb_reset_transport_header(skb); > > + WARN_ON(skb_headroom(skb) & 1); /* HW assumes word alignment */ > > + ph = (struct phonethdr *)skb_push(skb, sizeof(struct phonethdr)); > > + skb_reset_network_header(skb); > > Here you could just use: > > ph = (struct phonethdr *)skb_network_header(skb); > > or even just: > > ph = pn_hdr(skb); > > as other protocols such as IP do (see ip_hdr()) Right. > > + ph->rdev = pn_dev(dst); > > + ph->sdev = pn_dev(src); > > + ph->function = res; > > + ph->length = __cpu_to_be16(skb->len + 2 - sizeof(*ph)); > > + ph->robj = pn_obj(dst); > > + ph->sobj = pn_obj(src); > > + > > + skb->protocol = __constant_htons(ETH_P_PHONET); > > No need for __constant_htons(CONSTANT), htons will do the right thing > and the code will be shorter, clearer. Ok. There are quite many of these in net/ though... is that reserved for switch/case only? (...) > > +static int pn_ioctl(struct sock *sk, int cmd, unsigned long arg) > > +{ > > + struct sk_buff *skb; > > + int answ; > > + > > + switch (cmd) { > > + case SIOCINQ: > > + spin_lock_bh(&sk->sk_receive_queue.lock); > > + skb = skb_peek(&sk->sk_receive_queue); > > + answ = skb ? skb->len : 0; > > + spin_unlock_bh(&sk->sk_receive_queue.lock); > > + return put_user(answ, (int __user *)arg); > > Why not use lock_sock/release_sock as tcp? The socket lock was not taken on the RX path, as I was not using sk_receive_skb(). I believe no state needs protection from softirq in the datagram protocol, other than the receive queue itself. Of course, with sk_receive_skb(), I guess I could use lock_sock(). Thanks many times for the review, -- Rémi Denis-Courmont Maemo Software, Nokia Devices R&D -- 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