From patchwork Thu Apr 20 11:33:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Amir Goldstein X-Patchwork-Id: 9690251 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 03991600C8 for ; Thu, 20 Apr 2017 11:33:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F324C26E75 for ; Thu, 20 Apr 2017 11:33:43 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E80202845E; Thu, 20 Apr 2017 11:33:43 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1DD2826E75 for ; Thu, 20 Apr 2017 11:33:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031594AbdDTLdQ (ORCPT ); Thu, 20 Apr 2017 07:33:16 -0400 Received: from mail-oi0-f51.google.com ([209.85.218.51]:32993 "EHLO mail-oi0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S945252AbdDTLdG (ORCPT ); Thu, 20 Apr 2017 07:33:06 -0400 Received: by mail-oi0-f51.google.com with SMTP id y11so14538936oie.0; Thu, 20 Apr 2017 04:33:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=+74Ncnk1lLrX7lJYGrpbJyhIvCVBtKeXM4jnK8xAjWs=; b=e/xUO5+EQsdttji8+9wCM9oPZG6iX5kPLsDnPsXWeifdcA89UStiQ8YUcaZ/oHYiA5 7LRTc00GhQKpbUvQTe/QkPNMZl7+Pjo9emdi0Si8gpTKcC74DVNikcV7giNVz/pNW7Ev ZU2TIwoXU5baT4mLg7YDEN7RFQ5+6TzXkCe7ppkdh6Iqdb0Xd+1YnV8jUe1m6/mqsuHZ n4S/AbcBsrZMSUeLnu7FfXclIMccJqY8AbAhQ8XSwhuBu3s9MIeuQm4JrhLGj23kzXXT fytR2N9ivXYU+skqwJavi0vU3I9NJ8H1n8YAHo1wqUyyuEVEuZXuIM2jcrg9EBoDhupt Xw8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=+74Ncnk1lLrX7lJYGrpbJyhIvCVBtKeXM4jnK8xAjWs=; b=bZanAVAKwamiYXmvROxT/s78aZEPZqy7/Y8MwVJFSESSKSVxPVdeZCxMLjsjPO+owo u6/0Ji3iYQ7Qljx0fG8JmIEM8D+cSSTIghhMk98TCyQ/zeLNkW5ia24xqNcDhYX0aa8P IKW7QlSgJDDmRyJYsNYPe1jkDXgIycD8mMiG4A3rTNzHVB69pA8A02b3uMKr0rLpAsRW 7TiXnLZ1+SRLlXyPUnrdkjG9zIcNHK0pOsSFoxCv5cGbURNQ9QOSoEGkYbgc2TAkLkBN 8GKh2zU31iqIPAI+RbTZujmmsZw22ZTfBKlnY1SiU/tBX8xs+qLXWWn+NfqDbEkWPrE5 y0tA== X-Gm-Message-State: AN3rC/6jaZBZJS1PjieKUapXUuDy4oGhX8bVWL/8kTMjePAdiYPCgcYr Mg5rRAF1eS2hVK691eTvG5kQCP+nuQ== X-Received: by 10.202.173.147 with SMTP id w141mr306989oie.60.1492687985314; Thu, 20 Apr 2017 04:33:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.18.200 with HTTP; Thu, 20 Apr 2017 04:33:04 -0700 (PDT) In-Reply-To: References: <87inn12urq.fsf@drapion.f-secure.com> <20170322193122.GV29622@ZenIV.linux.org.uk> <87a88c2yxq.fsf@drapion.f-secure.com> <1490270212.3921.10.camel@poochiereds.net> <8760j02mfz.fsf@drapion.f-secure.com> <87lgqwa4tg.fsf@drapion.f-secure.com> From: Amir Goldstein Date: Thu, 20 Apr 2017 14:33:04 +0300 Message-ID: Subject: Re: fanotify read returns with errno == EOPENSTALE To: Marko Rauhamaa Cc: Jeff Layton , Al Viro , linux-fsdevel , Jan Kara , Linux NFS Mailing List , linux-api@vger.kernel.org Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Apr 20, 2017 at 2:06 PM, Amir Goldstein wrote: > On Wed, Apr 19, 2017 at 4:46 PM, Marko Rauhamaa > wrote: >> Amir Goldstein : >> >>> On Thu, Mar 23, 2017 at 8:43 AM, Marko Rauhamaa >>> wrote: >>>> Jeff Layton : >>>> >>>>> It was definitely not the intention to leak this error code to >>>>> userland. EOPENSTALE is not a POSIX sanctioned error code, so >>>>> applications generally don't know anything about it and will be >>>>> confused. >>>> >>>> Got it. I will try to work on a reproduction and make a proper bug >>>> report. >>> >>> Try this: >>> >>> - watch a single file for permissions events (so you will only have >>> one event in the queue) >>> - open file from client to generate single event (don't read event yet) >>> - remove file from server (to make it stale) >>> - read event (with stale file) >> >> I did that and reproduced the problem on a recent development kernel. >> Happens every time. >> >> Just take the example program listed under "man fanotify" ("fantest") >> and follow these steps: >> >> ============================================================== >> NFS Server NFS Client(1) NFS Client(2) >> ============================================================== >> # echo foo >/nfsshare/bar.txt >> # cat /nfsshare/bar.txt >> foo >> # ./fantest /nfsshare >> Press enter key to terminate. >> Listening for events. >> # rm -f /nfsshare/bar.txt >> # cat /nfsshare/bar.txt >> read: Unknown error 518 >> cat: /nfsshare/bar.txt: Operation not permitted >> ============================================================== >> >> where NFS Client (1) and (2) are two terminal sessions on a single NFS >> Client machine. >> > > Thanks for the reproducer. > I'll try it myself when I get to it. > >> So what do we conclude? Is this a kernel bug or works as designed? >> > > Exposing EOPENSTALE to userspace is definitely a kernel bug. > > >>> Oh my. I completely misread your report before. I though you were >>> trying to read from the event->fd. Now I understand that you mean read >>> from fanotify fd. That will definitely return the error, but only in >>> the special case where open error happened on the first event being >>> read to the buffer. If error happens after adding some events to the >>> buffer, fanotify process will not know about this. Regular event will >>> be silently dropped and permission event will be denied. >>> >>> [...] >>> >>> You do NOT need to call fanotify_init() again, the next read will read >>> the next event. >> >> It does appear that reading the fanotify fd again does the trick. >> >> However, the client gets an EPERM instead of ENOENT, which is a bit >> weird. >> > > Why would the client get ENOENT? That EOPENSTALE event is already > consumed, the client reads the next event in the queue. Sorry, I keep confusing when you refer to read of file and read of fanotify fd when kernel fails to get response from fanotify daemon it will deny access to file. That's the default. > >>> The fix seems trivial and I can post it once you have the test: >>> - return EAGAIN for read in case of a single event in queue without fd >>> so apps getting the read error will have a good idea what to do >>> - in case of non single event, maybe copy event with error on event->fd >>> to the buffer for specific errors that make sense to report (EMFILE) >>> so a watcher checks the values of negative event->fd can maybe do >>> something about it (e.g. provide a smaller buffer). >> >> EAGAIN would be perfect for me since I'm using fanotify in a nonblocking >> mode. It might be a bit surprising in the blocking case. >> >> > > Can you please try this patch? > Can you please try it with blocking and non-blocking > Can you please try to add to reproducer the non empty queue case: > - Add another mark on another mount without PERM events in the mask > - Populate other mount with some files > - Before reading from nfsshare, read from other mount to fill the > event queue, e.g.: > # cat /tmp/foo* /nfsshare/bar.txt /tmp/bar* > > This should result (depending on number of files) with >>= 2 buffer reads - first with /tmp/foo* files access > last with /tmp/bar* files access > > Sorry I messed up the previous patch. please try this one: * events can be destroyed now. @@ -305,7 +315,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, break; } else { #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (ret < 0) { + if (ret <= 0) { FANOTIFY_PE(kevent)->response = FAN_DENY; wake_up(&group->fanotify_data.access_waitq); break; --- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 2b37f27..7864354 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -295,6 +295,16 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, } ret = copy_event_to_user(group, kevent, buf); + if (unlikely(ret == -EOPENSTALE)) { + /* + * We cannot report events with stale fd so drop it. + * Setting ret to 0 will continue the event loop and + * do the right thing if there are no more events to + * read (i.e. return bytes read, -EAGAIN or wait). + */ + ret = 0; + } + /* * Permission events get queued to wait for response. Other