Hello Paolo Abeni,
The patch 419ce133ab92: "tcp: allow again tcp_disconnect() when
threads are waiting" from Oct 11, 2023 (linux-next), leads to the
following Smatch static checker warning:
net/ipv4/tcp_bpf.c:324 tcp_bpf_recvmsg_parser() warn: inconsistent returns '&sk->sk_lock.slock'.
net/ipv4/tcp_bpf.c:370 tcp_bpf_recvmsg() warn: inconsistent returns '&sk->sk_lock.slock'.
net/ipv4/tcp_bpf.c
218 static int tcp_bpf_recvmsg_parser(struct sock *sk,
219 struct msghdr *msg,
220 size_t len,
221 int flags,
222 int *addr_len)
223 {
224 struct tcp_sock *tcp = tcp_sk(sk);
225 int peek = flags & MSG_PEEK;
226 u32 seq = tcp->copied_seq;
227 struct sk_psock *psock;
228 int copied = 0;
229
230 if (unlikely(flags & MSG_ERRQUEUE))
231 return inet_recv_error(sk, msg, len, addr_len);
232
233 if (!len)
234 return 0;
235
236 psock = sk_psock_get(sk);
237 if (unlikely(!psock))
238 return tcp_recvmsg(sk, msg, len, flags, addr_len);
239
240 lock_sock(sk);
241
242 /* We may have received data on the sk_receive_queue pre-accept and
243 * then we can not use read_skb in this context because we haven't
244 * assigned a sk_socket yet so have no link to the ops. The work-around
245 * is to check the sk_receive_queue and in these cases read skbs off
246 * queue again. The read_skb hook is not running at this point because
247 * of lock_sock so we avoid having multiple runners in read_skb.
248 */
249 if (unlikely(!skb_queue_empty(&sk->sk_receive_queue))) {
250 tcp_data_ready(sk);
251 /* This handles the ENOMEM errors if we both receive data
252 * pre accept and are already under memory pressure. At least
253 * let user know to retry.
254 */
255 if (unlikely(!skb_queue_empty(&sk->sk_receive_queue))) {
256 copied = -EAGAIN;
257 goto out;
258 }
259 }
260
261 msg_bytes_ready:
262 copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
263 /* The typical case for EFAULT is the socket was gracefully
264 * shutdown with a FIN pkt. So check here the other case is
265 * some error on copy_page_to_iter which would be unexpected.
266 * On fin return correct return code to zero.
267 */
268 if (copied == -EFAULT) {
269 bool is_fin = is_next_msg_fin(psock);
270
271 if (is_fin) {
272 copied = 0;
273 seq++;
274 goto out;
275 }
276 }
277 seq += copied;
278 if (!copied) {
279 long timeo;
280 int data;
281
282 if (sock_flag(sk, SOCK_DONE))
283 goto out;
284
285 if (sk->sk_err) {
286 copied = sock_error(sk);
287 goto out;
288 }
289
290 if (sk->sk_shutdown & RCV_SHUTDOWN)
291 goto out;
292
293 if (sk->sk_state == TCP_CLOSE) {
294 copied = -ENOTCONN;
295 goto out;
296 }
297
298 timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
299 if (!timeo) {
300 copied = -EAGAIN;
301 goto out;
302 }
303
304 if (signal_pending(current)) {
305 copied = sock_intr_errno(timeo);
306 goto out;
307 }
308
309 data = tcp_msg_wait_data(sk, psock, timeo);
310 if (data < 0)
311 return data;
Do we need to call release_sock(sk); before returning? It gets called
in tcp_msg_wait_data() but then it calls lock_sock() again so Smatch
and I think that cancels out.
312 if (data && !sk_psock_queue_empty(psock))
313 goto msg_bytes_ready;
314 copied = -EAGAIN;
315 }
316 out:
317 if (!peek)
318 WRITE_ONCE(tcp->copied_seq, seq);
319 tcp_rcv_space_adjust(sk);
320 if (copied > 0)
321 __tcp_cleanup_rbuf(sk, copied);
322 release_sock(sk);
323 sk_psock_put(sk, psock);
--> 324 return copied;
325 }
regards,
dan carpenter
