From owner-svn-src-all@freebsd.org Sat Jan 27 15:17:01 2018 Return-Path: Delivered-To: svn-src-all@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 221D5EC6994 for ; Sat, 27 Jan 2018 15:17:01 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from sonic317-41.consmr.mail.bf2.yahoo.com (sonic317-41.consmr.mail.bf2.yahoo.com [74.6.129.96]) (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 BB6326C652 for ; Sat, 27 Jan 2018 15:17:00 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1517066214; bh=cYDrBZIeYAfPMc9HTVfIf/uFNYC5EYFoYG0wcI7h758=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From:Subject; b=MuXLhhTyQ2cDJA0jf2ZdV+UwyYgwOF511Ce//KSKjW3ghUJTU/kGI83B66aux+2cECDeUsd3+mOdYiFA7FirYUjZeB55KTgyHunTr3IkEiWDbv3pG5/rAIDhJsn0EEHQcRRz0quuI671dpAsKX+wUMe9hwYqqzWgYQV2QrPvZMRKGAtnB8UYLN+jeo50DQ6LA6CgEBoiqUx5MckDDIEoxnWDNYkmXKq158/OMnXFVpECf44CVECHqaeZXpnN4Q1wpsfruGSx8qwklJpcOBp2YYONPsZzXyWLz1VQOPPIwTda4Gny2oBx4ok9X1dJwGnL9kC7Gg3ZONHysVWpV05mDg== X-YMail-OSG: cTFeXBcVM1lEJvg2gcbCALACblaz7q4FhCSjMLGawfPSO7QOiLL.nmXn8S1fbZ8 cTXdgC1d1oAUZGySgz96JWszhpTvHzBFiNcTXH4WtU6yXhj.qh2KrEKhHvxWkC5F6Bms_Sw5A_qu GAyDjrcDo56HSvLxJr6KWp0OPnGawQG5fAceMJQr6eivWdCLnb94Nnoah_hDNJGvw.TTyoIp5o9h j6q5zztMHrQ1vVpLrSM_6Xg.9DipocHqWT79EO_FLKLqUCLxs.DqulXb4RM5F9WLIKUZm4Jf1u7d IN9sIqEjGEB3FqAQg0ViQ6B2chBX08MUNk71rGsf.7M1YA39pY3gYImZOfxix5qZg89McJdcO.r6 90nBAY9swt6rGibFUnFo2UukPlK3TPdLHOq6qM4L7IZ3w.WRdnFztCu3PWdGF9NzyTlWKFgLO7n5 yPfW4hfhDGsqbVcIEZ8u.naQnl6299XxSZegjvBaVOA1KhbN3Pe3aGlO1NBwPBTfmmzYtzu2Bk4n MXuEMZYsU8A-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic317.consmr.mail.bf2.yahoo.com with HTTP; Sat, 27 Jan 2018 15:16:54 +0000 Received: from smtpgate102.mail.bf1.yahoo.com (EHLO [192.168.0.8]) ([72.30.28.113]) by smtp409.mail.bf1.yahoo.com (JAMES SMTP Server ) with ESMTPA ID cc52141bb3269490e20db846c836f906; Sat, 27 Jan 2018 15:16:52 +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: <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org> Date: Sat, 27 Jan 2018 10:16:53 -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: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Jan 2018 15:17:01 -0000 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. > > 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, > 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. > > 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. > > 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. > Hmm ... We have never checked the upper bound there, which doesn't mean it was right. I found MAXPHYS, which seems a more reasonable limit used in the kernel for uio_resid. I am checking the patch compiles and doesn't give surprises. Pedro. > 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 >