| 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
| ||
|
Message-ID: <1342864435.25472.79.camel@haakon2.linux-iscsi.org>
Date: Sat, 21 Jul 2012 02:53:55 -0700
From: "Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To: Al Viro <viro@...IV.linux.org.uk>
Cc: David Miller <davem@...emloft.net>,
mikulas@...ax.karlin.mff.cuni.cz, eric.dumazet@...il.com,
maxk@...lcomm.com, vtun@...ice.satix.net, netdev@...r.kernel.org,
linux-sctp@...r.kernel.org, Andy Grover <agrover@...hat.com>,
Hannes Reinecke <mail@...nes-reinecke.de>,
Christoph Hellwig <hch@....de>,
Mike Christie <mchristi@...hat.com>,
target-devel <target-devel@...r.kernel.org>
Subject: Re: [PATCH] Crash in tun
On Sat, 2012-07-21 at 08:55 +0100, Al Viro wrote:
> BTW, speaking of struct file treatment related to sockets -
> there's this piece of code in iscsi:
> /*
> * The SCTP stack needs struct socket->file.
> */
> if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
> (np->np_network_transport == ISCSI_SCTP_UDP)) {
> if (!new_sock->file) {
> new_sock->file = kzalloc(
> sizeof(struct file), GFP_KERNEL);
>
> For one thing, as far as I can see it'not true - sctp does *not* depend on
> socket->file being non-NULL; it does, in one place, check socket->file->f_flags
> for O_NONBLOCK, but there it treats NULL socket->file as "flag not set".
> Which is the case here anyway - the fake struct file created in
> __iscsi_target_login_thread() (and in iscsi_target_setup_login_socket(), with
> the same excuse) do *not* get that flag set.
>
> Moreover, it's a bloody serious violation of a bunch of asserts in VFS;
> all struct file instances should come from filp_cachep, via get_empty_filp()
> (or alloc_file(), which is a wrapper for it). FWIW, I'm very tempted to
> do this and be done with the entire mess:
>
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> ---
<nod> Merged into target-pending.git/for-next.
For the record, this logic was originally required in order to get non
multi-homed SCTP endpoints with iscsi_target_mod to connect using
Core-iSCSI/SCTP initiators to Linux/SCTP, but it's obviously incorrect
for modern code.
Since we don't have iscsi_sctp for Open/iSCSI code implemented just yet
(mnc CC'ed), adding CC' to drop this incorrect code for stable.
Thank you,
--nab
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index d57d10c..d7dcd67 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -429,19 +429,8 @@ int iscsit_reset_np_thread(
>
> int iscsit_del_np_comm(struct iscsi_np *np)
> {
> - if (!np->np_socket)
> - return 0;
> -
> - /*
> - * Some network transports allocate their own struct sock->file,
> - * see if we need to free any additional allocated resources.
> - */
> - if (np->np_flags & NPF_SCTP_STRUCT_FILE) {
> - kfree(np->np_socket->file);
> - np->np_socket->file = NULL;
> - }
> -
> - sock_release(np->np_socket);
> + if (np->np_socket)
> + sock_release(np->np_socket);
> return 0;
> }
>
> @@ -4077,13 +4066,8 @@ int iscsit_close_connection(
> kfree(conn->conn_ops);
> conn->conn_ops = NULL;
>
> - if (conn->sock) {
> - if (conn->conn_flags & CONNFLAG_SCTP_STRUCT_FILE) {
> - kfree(conn->sock->file);
> - conn->sock->file = NULL;
> - }
> + if (conn->sock)
> sock_release(conn->sock);
> - }
> conn->thread_set = NULL;
>
> pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
> diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
> index 1c70144..1dd5716 100644
> --- a/drivers/target/iscsi/iscsi_target_core.h
> +++ b/drivers/target/iscsi/iscsi_target_core.h
> @@ -224,7 +224,6 @@ enum iscsi_timer_flags_table {
> /* Used for struct iscsi_np->np_flags */
> enum np_flags_table {
> NPF_IP_NETWORK = 0x00,
> - NPF_SCTP_STRUCT_FILE = 0x01 /* Bugfix */
> };
>
> /* Used for struct iscsi_np->np_thread_state */
> @@ -503,7 +502,6 @@ struct iscsi_conn {
> u16 local_port;
> int net_size;
> u32 auth_id;
> -#define CONNFLAG_SCTP_STRUCT_FILE 0x01
> u32 conn_flags;
> /* Used for iscsi_tx_login_rsp() */
> u32 login_itt;
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index a3656c9..ae30424 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -795,22 +795,6 @@ int iscsi_target_setup_login_socket(
> }
> np->np_socket = sock;
> /*
> - * The SCTP stack needs struct socket->file.
> - */
> - if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
> - (np->np_network_transport == ISCSI_SCTP_UDP)) {
> - if (!sock->file) {
> - sock->file = kzalloc(sizeof(struct file), GFP_KERNEL);
> - if (!sock->file) {
> - pr_err("Unable to allocate struct"
> - " file for SCTP\n");
> - ret = -ENOMEM;
> - goto fail;
> - }
> - np->np_flags |= NPF_SCTP_STRUCT_FILE;
> - }
> - }
> - /*
> * Setup the np->np_sockaddr from the passed sockaddr setup
> * in iscsi_target_configfs.c code..
> */
> @@ -869,21 +853,15 @@ int iscsi_target_setup_login_socket(
>
> fail:
> np->np_socket = NULL;
> - if (sock) {
> - if (np->np_flags & NPF_SCTP_STRUCT_FILE) {
> - kfree(sock->file);
> - sock->file = NULL;
> - }
> -
> + if (sock)
> sock_release(sock);
> - }
> return ret;
> }
>
> static int __iscsi_target_login_thread(struct iscsi_np *np)
> {
> u8 buffer[ISCSI_HDR_LEN], iscsi_opcode, zero_tsih = 0;
> - int err, ret = 0, set_sctp_conn_flag, stop;
> + int err, ret = 0, stop;
> struct iscsi_conn *conn = NULL;
> struct iscsi_login *login;
> struct iscsi_portal_group *tpg = NULL;
> @@ -894,7 +872,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
> struct sockaddr_in6 sock_in6;
>
> flush_signals(current);
> - set_sctp_conn_flag = 0;
> sock = np->np_socket;
>
> spin_lock_bh(&np->np_thread_lock);
> @@ -917,35 +894,12 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
> spin_unlock_bh(&np->np_thread_lock);
> goto out;
> }
> - /*
> - * The SCTP stack needs struct socket->file.
> - */
> - if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
> - (np->np_network_transport == ISCSI_SCTP_UDP)) {
> - if (!new_sock->file) {
> - new_sock->file = kzalloc(
> - sizeof(struct file), GFP_KERNEL);
> - if (!new_sock->file) {
> - pr_err("Unable to allocate struct"
> - " file for SCTP\n");
> - sock_release(new_sock);
> - /* Get another socket */
> - return 1;
> - }
> - set_sctp_conn_flag = 1;
> - }
> - }
> -
> iscsi_start_login_thread_timer(np);
>
> conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
> if (!conn) {
> pr_err("Could not allocate memory for"
> " new connection\n");
> - if (set_sctp_conn_flag) {
> - kfree(new_sock->file);
> - new_sock->file = NULL;
> - }
> sock_release(new_sock);
> /* Get another socket */
> return 1;
> @@ -955,9 +909,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
> conn->conn_state = TARG_CONN_STATE_FREE;
> conn->sock = new_sock;
>
> - if (set_sctp_conn_flag)
> - conn->conn_flags |= CONNFLAG_SCTP_STRUCT_FILE;
> -
> pr_debug("Moving to TARG_CONN_STATE_XPT_UP.\n");
> conn->conn_state = TARG_CONN_STATE_XPT_UP;
>
> @@ -1205,13 +1156,8 @@ old_sess_out:
> iscsi_release_param_list(conn->param_list);
> conn->param_list = NULL;
> }
> - if (conn->sock) {
> - if (conn->conn_flags & CONNFLAG_SCTP_STRUCT_FILE) {
> - kfree(conn->sock->file);
> - conn->sock->file = NULL;
> - }
> + if (conn->sock)
> sock_release(conn->sock);
> - }
> kfree(conn);
>
> if (tpg) {
--
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