Messages in this thread | | | Date | Sun, 12 Apr 2020 22:49:14 -0700 | From | Vito Caputo <> | Subject | Re: kernel BUG at net/phonet/socket.c:LINE! |
| |
On Sat, Apr 11, 2020 at 10:36:59AM +0300, Rémi Denis-Courmont wrote: > Hi, > > Le lauantaina 11. huhtikuuta 2020, 3.43.20 EEST Vito Caputo a écrit : > > I stared a bit at the code surrounding this report, and maybe someone more > > familiar with the network stack can clear something up for me real quick: > > > RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] > > > > net/phonet/socket.c: > > 202 static int pn_socket_autobind(struct socket *sock) > > 203 { > > 204 struct sockaddr_pn sa; > > 205 int err; > > 206 > > 207 memset(&sa, 0, sizeof(sa)); > > 208 sa.spn_family = AF_PHONET; > > 209 err = pn_socket_bind(sock, (struct sockaddr *)&sa, > > 210 sizeof(struct sockaddr_pn)); > > 211 if (err != -EINVAL) > > 212 return err; > > 213 BUG_ON(!pn_port(pn_sk(sock->sk)->sobject)); > > 214 return 0; /* socket was already bound */ > > 215 } > > > > line 213 is the BUG_ON for !sock->sk->sobject, a phonet-specific member: > > > > include/net/phonet/phonet.h: > > 23 struct pn_sock { > > 24 struct sock sk; > > 25 u16 sobject; > > 26 u16 dobject; > > 27 u8 resource; > > 28 }; > > > > pn_socket_autobind() expects that to be non-null whenever pn_socket_bind() > > returns -EINVAL, which seems odd, but the comment claims it's already bound, > > let's look at pn_socket_bind(): > > > > net/phonet/socket.c: > > 156 static int pn_socket_bind(struct socket *sock, struct sockaddr *addr, > > int len) 157 { > > 158 struct sock *sk = sock->sk; > > 159 struct pn_sock *pn = pn_sk(sk); > > 160 struct sockaddr_pn *spn = (struct sockaddr_pn *)addr; > > 161 int err; > > 162 u16 handle; > > 163 u8 saddr; > > 164 > > 165 if (sk->sk_prot->bind) > > 166 return sk->sk_prot->bind(sk, addr, len); > > 167 > > 168 if (len < sizeof(struct sockaddr_pn)) > > 169 return -EINVAL; > > 170 if (spn->spn_family != AF_PHONET) > > 171 return -EAFNOSUPPORT; > > 172 > > 173 handle = pn_sockaddr_get_object((struct sockaddr_pn *)addr); > > 174 saddr = pn_addr(handle); > > 175 if (saddr && phonet_address_lookup(sock_net(sk), saddr)) > > 176 return -EADDRNOTAVAIL; > > 177 > > 178 lock_sock(sk); > > 179 if (sk->sk_state != TCP_CLOSE || pn_port(pn->sobject)) { > > 180 err = -EINVAL; /* attempt to rebind */ > > 181 goto out; > > 182 } > > 183 WARN_ON(sk_hashed(sk)); > > 184 mutex_lock(&port_mutex); > > 185 err = sk->sk_prot->get_port(sk, pn_port(handle)); > > 186 if (err) > > 187 goto out_port; > > 188 > > 189 /* get_port() sets the port, bind() sets the address if > > applicable */ 190 pn->sobject = pn_object(saddr, > > pn_port(pn->sobject)); > > 191 pn->resource = spn->spn_resource; > > 192 > > 193 /* Enable RX on the socket */ > > 194 err = sk->sk_prot->hash(sk); > > 195 out_port: > > 196 mutex_unlock(&port_mutex); > > 197 out: > > 198 release_sock(sk); > > 199 return err; > > 200 } > > > > > > The first return branch in there simply hands off the bind to and > > indirection sk->sk_prot->bind() if present. This smells ripe for breaking > > the assumptions of that BUG_ON(). > > I believe that this is in line with the design of the socket stack within the > Linux kernel. 'struct proto_ops' carries the protocol family operations, then > 'struct proto' carries the protocol operations. > > Admittedly, Phonet only had one datagram and one stream protocol ever written, > as the hardware development ceased. So in practice, there is a 1:1 mapping > between the two, and sk_prot.bind is always NULL. > > > I'm assuming there's no point for such an indirection if not to enable a > > potentially non-phonet-ops hook, otherwise we'd just be do the bind. > > In my understanding, that's *not* what sk_prot is for, no. It's rather meant > to specialize the socket calls on a per-protocol basis. > > For instance, UDP and UDP-Lite share their address family 'struct proto_ops' > (either inet_dgram_ops or inet6_dgram_ops) but they have different > 'struct proto'. > > > If not, isn't this plain recursive? Color me confused. Will other bind() > > implementations return -EINVAL when already bound with sobject set? no. > > As far as I can find, there are no cases where the bind pointer would not be > NULL in upstream kernel. This can only happen if an out-of-tree module defines > its own protocol within the Phonet family. >
Ok, so we can disregard that bit as benign apparently.
> > Furthermore, -EINVAL is also returned when len < sizeof(struct sockaddr_pn), > > maybe the rebind case should return -EADDRINUSE instead of -EINVAL? > > bind() semantics require returning EINVAL if the socket address size is, well, > invalid. > > If we are to distinguish the two error scenarii, then it's the rebind case > that needs a different error, but EINVAL is consistent with INET. >
Isn't the existing code is bugged if treating -EINVAL as valid and a rebind?
The invalid size will return a NULL sobject but -EINVAL, triggering the BUG_ON.
Thanks, Vito Caputo
| |