From owner-svn-src-head@freebsd.org Fri Jan 26 18:43:28 2018 Return-Path: Delivered-To: svn-src-head@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 4CDEDEC2AB5 for ; Fri, 26 Jan 2018 18:43:28 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from sonic304-35.consmr.mail.ne1.yahoo.com (sonic304-35.consmr.mail.ne1.yahoo.com [66.163.191.161]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DA9FB818DE for ; Fri, 26 Jan 2018 18:43:27 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1516992201; bh=34glKoewZFX/+SxFiDNcehPZGAn7FM2eRPgnjyYt6lQ=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From:Subject; b=JsO+3UgDkkySei6pOVh9fNu/Uln/xlT9jPvaoBdo7TczbvcvMxabE/Bh7KKxPSss2H5BS7V9oXOUJPXxG8YsiVTfx583vcLQ/XRrYgu7DwlMGBzyhrzCxmsIbGgn41++NADmjx/5VkO1gzW5YWZ85Fa/w/lXwEt69RZrB1YsdgjDTMEyvuz78OBXmfAdSuzL4wbaiIdv3UqlGvCbSBc5Ibuww+ZOMiaHoC6big40ni2EHVpVX6lh00OnisaLJ9kPONMcUc2CLMSs1x0Iw+O2Bq8P8S0regZGUyq7X2xBdYDHepP/c0byiETOpk7zhAPnaYRoSpMte5DCUJJGwfTjUQ== X-YMail-OSG: pz23J5EVM1lRAih4g_LiHJSdvFplFb2kooTe6TTSSotp6VvxxtmNeBnl3XG2Y6V tDKF0pDaCDb190PtdOOaENtqHsp6wxjV1oBGobFalL8jFGPw_UncMPBlDyXOspgekMbN4csUTImh ILvZZ.tcLNlSA1sF8MjNv8TS02ihKhlKzP3Jr.jJ6ooTmKiwdYYaBqQDuM7iOHaqYcnB6__2cbCl AOBcwzvAmPJ3VEq5oUcYClaHsmp3ccSFK0muWn8jX.F6IXZB2aV.q0TYiveGxgtAQdXYoz6XhnSZ h0nGt0WTCHLSgWa3sgZaebYf8zdOtGXKYP9T8fgpB1NRov6x3L3iqIPlk7l_nePZJTYN_oWdQbqf XwCd6EtjX3BKxOtjr4KMCN5eMB.UDAXJj.4MjCfJBN.XQ9OYE5N.esk4Veqv8xVmSxfRIfEYuGrI 1ODWgGn6Rcdel889xroYT4fR1Qzqb.mVxTxappgq8lWhrgTnvXw5DrOzx4ofTYWH07X6bMwokNw1 GxQK4OZPqww-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic304.consmr.mail.ne1.yahoo.com with HTTP; Fri, 26 Jan 2018 18:43:21 +0000 Received: from smtp233.mail.ne1.yahoo.com (EHLO [192.168.0.8]) ([10.218.253.208]) by smtp411.mail.ne1.yahoo.com (JAMES SMTP Server ) with ESMTPA ID 269995b754cd839cff391d29ff87b5af; Fri, 26 Jan 2018 18:43:18 +0000 (UTC) Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs To: Bruce Evans Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201801241758.w0OHwm26063524@repo.freebsd.org> <20180126020540.B2181@besplex.bde.org> <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> <20180126053133.R3207@besplex.bde.org> <21b6bdda-65b7-89da-4dd6-bed64978eba8@FreeBSD.org> <20180126214948.C1040@besplex.bde.org> From: Pedro Giffuni Message-ID: <10edbded-b2b5-07e8-b527-b9b4c46d0bae@FreeBSD.org> Date: Fri, 26 Jan 2018 13:43:16 -0500 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180126214948.C1040@besplex.bde.org> Content-Type: multipart/mixed; boundary="------------CD1214954B5C6366568C4C97" Content-Language: en-US X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Jan 2018 18:43:28 -0000 This is a multi-part message in MIME format. --------------CD1214954B5C6366568C4C97 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 01/26/18 06:36, Bruce Evans wrote: > On Thu, 25 Jan 2018, Pedro Giffuni wrote: > >> On 25/01/2018 14:24, Bruce Evans wrote: >>> ... >>> This code only works because (if?) nfs is the only caller and nfs never >>> passes insane values. >>> >> >> I am starting to think that we should simply match uio_resid and set >> it to ssize_t. >> Returning the value to int is certainly not the solution. > > Of course using the correct type (int) is part of the solution. > int *was* the correct type, now it doesn't cover all the range. > uio_must be checked before it is used for cookies, and after checking > it, it > is small so it fits easily in an int.  It must also checked to be > nonnegative, Our problem is not really uio_*, our problem is ncookies and we only test that it is >0. I think the attached patch, still keeping the unsigned ncookies, is sufficient. > > so that it doesn't suffer unsigned poisoning when it is promoted, so > it would > also fit in a u_int, but using u_int to store it is silly as using 1U > instead > of 1 for a count of 1. > > The bounds checking is something like: > >     if (ap->uio_resid < 0) >         ap->uio_resid = 0; >     if (ap->a_ncookies != NULL) { >         if (ap->uio_resid >= 64 * 1024) >             ap->uio_resid = 64 * 1024; >         ncookies = ap->uio_resid; >     } > > This checks for negative values for all cases and converts to 0 (EOF) to > preserve historical behaviour for the syscall case and to avoid overflow > for the cookies case (in case the caller is buggy).  The correct handling > is to return EINVAL, but EOF is good enough. > This also touches uio_resid which is probably not good as it is used later on. Our job is not to "fix" the caller but only to apply a reasonable behavior. I also don't like the magic numbers here. > In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it > or corrupt it by assigning it to an int or u_int. > The minimal type conversion does not really involve corruption: ncookies is local and the caller will not perceive any change. Pedro. > Limit uio_resid from above only in the cookies case.  The final limit > should > be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above > the limit, since nfs probably wouldn't know how to handle that (by > retrying > with a smaller size).  Test its handling of short counts instead. It is > expected than nfs asks for 128K and we supply at most 64K.  The supply is > always reduced at EOF.  Hopefully nfs doesn't treat the short count as > EOF. > It should retry until we supply 0. > > After limiting uio_resid, assign it to the int ncookies. > > This doesn't fix the abuse of the ncookies counter to hold the size of > the > cookies array in bytes for this and the next couple of statements. > > Normally the bounds checking should be at the top level, with at most > KASSERT()s at lower levels, but here the levels are mixed, and it isn't > clear if kernel callers have already checked, and it doesn't cost much > to do much the same checking for the kernel callers as for the syscall > callers. > > Perhaps the 128K limit is good for all cases (this depends on callers not > having buggy short count handling).  Directories of this size are very > rare (don't forget to create very large ones when you test this). Doing > anything with directories of this size tends to be slow anyway, and the > slowness has nothing to do with reading only 128K instead of SSIZE_MAX > bytes at a time. > > readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except > in the unionfs case it seems to try to read the whole direction. It > malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing > a buffer large enough for a whole file or directory is no good, since > in theory even directory sizes can be much larger than memory. > > Bruce --------------CD1214954B5C6366568C4C97 Content-Type: text/x-patch; name="ext2_ncookies.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="ext2_ncookies.diff" Index: sys/fs/ext2fs/ext2_lookup.c =================================================================== --- sys/fs/ext2fs/ext2_lookup.c (revision 328443) +++ sys/fs/ext2fs/ext2_lookup.c (working copy) @@ -153,7 +153,10 @@ return (EINVAL); ip = VTOI(vp); if (ap->a_ncookies != NULL) { - ncookies = uio->uio_resid; + if (uio->uio_resid < 0) + ncookies = 0; + else + ncookies = uio->uio_resid; if (uio->uio_offset >= ip->i_size) ncookies = 0; else if (ip->i_size - uio->uio_offset < ncookies) --------------CD1214954B5C6366568C4C97--