linux-kernel - Re: [PATCH] iscsi-target: Fail connection on short writes/reads

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: <20141216060417.GY22149@ZenIV.linux.org.uk>
Date:	Tue, 16 Dec 2014 06:04:17 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	"Nicholas A. Bellinger" <nab@...erainc.com>
Cc:	target-devel <target-devel@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Nicholas Bellinger <nab@...ux-iscsi.org>
Subject: Re: [PATCH] iscsi-target: Fail connection on short writes/reads

On Tue, Dec 16, 2014 at 04:48:58AM +0000, Nicholas A. Bellinger wrote:

> In practice this has not been an issue because iscsit_do_tx_data()
> is only used for transferring 48 byte headers + 4 byte digests,
> along with seldom used control payloads from NOPIN + TEXT_RSP +
> REJECT with less than 32k of data.  Nor has it been occuring with
> iscsit_do_rx_data() because MSG_WAITALL won't return to caller
> until the requested transfer length is reached, or an error has
> occured.
> 
> So following Al's audit of iovec consumers, go ahead and fail
> the connection on short writes/reads for now, and remove the
> bogus logic.

Umm...  This won't apply anymore.  For one thing, rscvmsg path in mainline
doesn't use kernel_recvmsg() - not since
commit e5a4b0bb803b39a36478451eae53a880d2663d5b
Author: Al Viro <viro@...iv.linux.org.uk>
Date:   Mon Nov 24 18:17:55 2014 -0500

    switch memcpy_to_msg() and skb_copy{,_and_csum}_datagram_msg() to primitives

That code has already become
        memset(&msg, 0, sizeof(struct msghdr));
        iov_iter_kvec(&msg.msg_iter, READ | ITER_KVEC,
                      count->iov, count->iov_count, data);

        while (total_rx < data) {
                rx_loop = sock_recvmsg(conn->sock, &msg,
                                      (data - total_rx), MSG_WAITALL);
                if (rx_loop <= 0) {
                        pr_debug("rx_loop: %d total_rx: %d\n",
                                rx_loop, total_rx);
                        return rx_loop;
                }
                total_rx += rx_loop;
                pr_debug("rx_loop: %d, total_rx: %d, data: %d\n",
                                rx_loop, total_rx, data);
        }

with short reads dealt with just fine - we set ->msg_iter once and each
call of sock_recvmsg() (which doesn't need set_fs() anymore) advances
it for the amount actually received.

sendmsg() side is trivially dealt with in the same fashion.  I haven't
pushed that into vfs#iov_iter-net yet, but as soon as the davem opens
net-next I'll be posting the sendmsg part of the series for review and
this will go there as well.  FWIW, right now it looks thus:

        iov_iter_kvec(&msg.msg_iter, WRITE | ITER_KVEC,
                      count->iov, count->iov_count, data);
        while (msg_data_left(&msg)) {
                tx_loop = sock_sendmsg(conn->sock, &msg);
                if (tx_loop <= 0) {
                        pr_debug("tx_loop: %d total_tx %d\n",
                                tx_loop, total_tx);
                        return tx_loop;
                }
                total_tx += tx_loop;
                pr_debug("tx_loop: %d, total_tx: %d, data: %d\n",
                                        tx_loop, total_tx, data);
        }

(msg_data_left(msg) == iov_iter_count(&msg->msg_iter) and sock_sendmsg()
has lost the third argument - it was always equal to msg_data_left(msg)).

iovec is never drained, ->msg_iter is always advanced by the amount actually
sent.  Makes (ex-)users of kernel_sendmsg()/kernel_recvmsg() much simpler
and trivial way of handling short writes/reads becomes correct...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/