From owner-freebsd-hackers@freebsd.org Fri Sep 21 19:22:12 2018 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 70B0310A1CEF for ; Fri, 21 Sep 2018 19:22:12 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com [IPv6:2607:f8b0:4864:20::d2c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 10D1E8CCF5 for ; Fri, 21 Sep 2018 19:22:11 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-io1-xd2c.google.com with SMTP id y3-v6so13257217ioc.5 for ; Fri, 21 Sep 2018 12:22:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XkdSF852DSIiArO9DHc8b4GLUjSsj0qpCt+BgSSM0Qo=; b=g1QDKLrAFWB7hOGmZZXSmLwa2klyD1REHKZPQ1TDuF7X9Ieqkruq2J2pNspFkTKqT/ XIpsOFNOEhgRUyZxunI5wGv+MIPBCJq0QPgqWJy0ozS0xS0Ix/YYiCIQK1Zl1T30Mu3X G8DAgq0JJEIA7E3klaibLT3rLqpY2Tcq6bGfldz4C2beoMO6jRShuUC+XXLMpWxkNMM2 u8VNr9CAyG+CCfsN9dFn4cwHNwOYbBrzDwrZAiUPPco1BqTUxKYpXAnVgYszpceax8Aa I9IX61UNNaIvr2YKBdRTuoHpMpH8pRsBTGZejU95vR4fjxvhfjPMk2BC05c32NgWF+Po 3+LQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XkdSF852DSIiArO9DHc8b4GLUjSsj0qpCt+BgSSM0Qo=; b=Wb3P632aOPpyAf8/Tfgl2oPYGfOzDj/gBFmv5smyFFK7xNmcPr4Av3xlzJ9+w+dHSn DOAjwqycfdO+PfqVyzZ+kz+K2P51zTwSAY6dnJnEVEy6T+sPD+EJGLpqCkKkTmhOD+C6 V/1ZBnG0DVjNmqRGCMEeG0ZabAQDqlvUfrTvjEKjCb0MKsfWZLBRHJ4UU87NaPlXZ99z b75D1RUidtaYxFjHJdGaAeCHYFqc1rU09CEakJO6IQXqJmbcvIqELYotj08nfTz/Cx3x 8Z5bWrgUTIJ4EO1PoJr5mNO/VZ3s6uCJvE5k9flMS/wvaKtYFyJabgP4dpG2WYmVvsvB vAXQ== X-Gm-Message-State: APzg51A/P/le+nr91DgULZUfyZzAJHzx0VfcRT9B+2CbI/lmTak/x9xb UIE++2jCSUaN1BArTLUxCR+ReWammEhIUEr9rbRD3w== X-Google-Smtp-Source: ANB0VdaRPaOgyaIjo7w2dnunftJhm2ODNtyRTuWGFcXtui0QnLyMjOkGbPc68urmcOzXZ5aDMDkXHYC/ORteQDEfxcc= X-Received: by 2002:a6b:3902:: with SMTP id g2-v6mr38283050ioa.168.1537557730848; Fri, 21 Sep 2018 12:22:10 -0700 (PDT) MIME-Version: 1.0 References: <3EBF1660-6CD5-4C80-A36D-4DE945073006@yahoo.com> <3E0252F3-3E65-4A2B-B17A-3BBFBAFFD5F6@yahoo.com> In-Reply-To: <3E0252F3-3E65-4A2B-B17A-3BBFBAFFD5F6@yahoo.com> From: Warner Losh Date: Fri, 21 Sep 2018 13:21:59 -0600 Message-ID: Subject: Re: Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16) make any sense? To: Mark Millard Cc: "freebsd-hackers@freebsd.org" Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.27 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Sep 2018 19:22:12 -0000 On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers < freebsd-hackers@freebsd.org> wrote: > [I decided that freebsd-hackers might be better for this, > under a different wording.] > > sys/dev/fxp/if_fxp.c uses the statement: > > cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16); > > But sys/dev/fxp/if_fxpreg.h has the types involved as: > > struct fxp_cb_tx { > uint16_t cb_status; > uint16_t cb_command; > uint32_t link_addr; > uint32_t tbd_array_addr; > uint16_t byte_count; > uint8_t tx_threshold; > uint8_t tbd_number; > > /* > * The following structure isn't actually part of the TxCB, > * unless the extended TxCB feature is being used. In this > * case, the first two elements of the structure below are > * fetched along with the TxCB. > */ > union { > struct fxp_ipcb ipcb; > struct fxp_tbd tbd[FXP_NTXSEG + 1]; > } tx_cb_u; > }; > > So cbp->tbd is not pointing into the middle of an array. > Thus the cbp->tbd[-1].tb_size = . . . assignment trashes memory > from what I can tell. > > /usr/src/sys/dev/fxp/if_fxp.c has the [-1] assignment in: > > /* Configure TSO. */ > if (m->m_pkthdr.csum_flags & CSUM_TSO) { > cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16); > cbp->tbd[1].tb_size |= htole32(tcp_payload << 16); > cbp->ipcb_ip_schedule |= FXP_IPCB_LARGESEND_ENABLE | > FXP_IPCB_IP_CHECKSUM_ENABLE | > FXP_IPCB_TCP_PACKET | > FXP_IPCB_TCPUDP_CHECKSUM_ENABLE; > } > > This cbp->tbd[-1].tb_size use goes back to -r185330 in 2008-Nov-26. > > This is also when the "+ 1" was added to the: > > struct fxp_tbd tbd[FXP_NTXSEG + 1] > > above. > > clang 7 via xtoolchain-llvm70 reported: > > --- if_fxp.o --- > /usr/src/sys/dev/fxp/if_fxp.c:1630:3: error: array index -1 is before the > beginning of the array [-Werror,-Warray-bounds] > cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16); > ^ ~~ > /usr/src/sys/dev/fxp/if_fxpreg.h:297:3: note: array 'tbd' declared here > struct fxp_tbd tbd[FXP_NTXSEG + 1]; > ^ > 1 error generated. > *** [if_fxp.o] Error code 1 > > It does look odd to me. > So I did some digging into this a few months ago. It turns out the code is right, kinda. So, what's it's doing with the [-1] is as follows: the sizeof tbd is 8 bytes long. So, we're dereferencing 8 before the array, which points to tbd_array_addr. However tb_size is the second element of that, so that points us at count + tx_threshold + tbd_number. So, this code is setting count = 0 and tx_threshold to the low order bits of the segment size and tbd_number to the high order bits. We set the count = 0 later anyway, so that part of it isn't so bad. Since this is in hardware, it may be special meanings for these bits, and this 'trick' is used to just do one write rather than two. I've not looked for the hardware manual to see if this is kosher, but that's what we're doing. It's not as crazy stupid as it sounds at first blush. Warner