From owner-svn-src-all@freebsd.org Sat Jan 27 16:14:14 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 A5F42EC9553 for ; Sat, 27 Jan 2018 16:14:14 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 3A8AC6E70F for ; Sat, 27 Jan 2018 16:14:14 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-it0-x241.google.com with SMTP id k131so3741773ith.4 for ; Sat, 27 Jan 2018 08:14:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=qsxt3V3oPqVbCKWYnMyHaTw2Zp4bd7rO/muXpG4hxYk=; b=jk+AZSwISYxGA/CtPTJySx+BZbYr0fp2dSuXZhc37JjhpCFgU9HuPpueuaL+8Cap36 vgrDU5HxT/G++HzdkqFy203BezRjYdTUYhGmCzgMWTiKFxG10c+qHmwMlEf2PInWvczI 8TZb3xatV1WgPXsfo4samzl7u3JgYic7BwaHqwN2B3PEnxdIoYg3zeAIIb6IPtNcVvws NsvLYJVDYUyiVYnIxVqlJPHF5a6V9yYV7wK5ZUkDfk43lg1O3iSmfqE6CADx1qz4HIoV Q0L49znPha1iIktxLgMEbpm2mikT1moPNl3w3/4dcIeQHlu1wzdVGXJvjomnaoduYs14 HIQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=qsxt3V3oPqVbCKWYnMyHaTw2Zp4bd7rO/muXpG4hxYk=; b=XATpnKcIk8PWohVkQ5yeG8C+pb3VkbF90w9wIsqrj//waEG8ncUethAtxrT3DsmUpb PmLwbIZcz9IsEcGCX2ML6FeSXuG6JR0zJT52PP4de+O/zjvHJcq5kMTb3+E7qGxyrdpE pwuqCqYshQlFMrlILXa5lHuly/qJz2BN0FUhe405ngcy//Tr76boRL7h0YsJkjh+So0u wUsLfMilNMp76O6a7yGGZvWPYOIThoMoaXvWW4ybh36bPnN7Zj/qDgl5JQbY9KNdTyKH zOoJHi6XKCxASXe2O5jTDmtzNZN1EY9TIX5gIsdpHUWnhA6eaL2MW1RXYmeA7O4K8IyL Hf2w== X-Gm-Message-State: AKwxyte08u4lyzes1SLyIm0sb/7LYEuKHiixiGUq6qtQ7EWfT8Ig99sR /InJRkpNSBk5iUad1+WWBxihhR6PykC7GkxyMpIUsw== X-Google-Smtp-Source: AH8x224wILiU9Z5iDXhN+CnVwlIRGPZy0OAGF77N1gk67dfOsAiI/4OGLKYuvbMW5TGtl14EKqwLfzJekSYwIJQAso0= X-Received: by 10.36.146.196 with SMTP id l187mr21723386itd.115.1517069653304; Sat, 27 Jan 2018 08:14:13 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.79.201.67 with HTTP; Sat, 27 Jan 2018 08:14:11 -0800 (PST) X-Originating-IP: [2607:fb90:6e36:93d6:c84:a4d3:7333:a790] Received: by 10.79.201.67 with HTTP; Sat, 27 Jan 2018 08:14:11 -0800 (PST) In-Reply-To: <11937120-bbb4-5da1-f48c-240a6aeafbd9@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> <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org> From: Warner Losh Date: Sat, 27 Jan 2018 09:14:11 -0700 X-Google-Sender-Auth: bZvLvykL6lFGkfwiu-p6P0M0bt0 Message-ID: Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs To: Pedro Giffuni Cc: Bruce Evans , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 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 16:14:14 -0000 On Jan 27, 2018 8:17 AM, "Pedro Giffuni" wrote: 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. MAXPHYS is almost the right thing to check. There's per device limits for normal I/O, but that doesn't seem to be a strict limit for readdir. Warner 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 > >