From patchwork Thu Apr 20 11:06:12 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Amir Goldstein X-Patchwork-Id: 9690111 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 D840060383 for ; Thu, 20 Apr 2017 11:06:30 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B131B28387 for ; Thu, 20 Apr 2017 11:06:30 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A604D28464; Thu, 20 Apr 2017 11:06:30 +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=unavailable 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 3B46C28387 for ; Thu, 20 Apr 2017 11:06:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966670AbdDTLGQ (ORCPT ); Thu, 20 Apr 2017 07:06:16 -0400 Received: from mail-oi0-f41.google.com ([209.85.218.41]:34957 "EHLO mail-oi0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965403AbdDTLGN (ORCPT ); Thu, 20 Apr 2017 07:06:13 -0400 Received: by mail-oi0-f41.google.com with SMTP id j201so45581064oih.2; Thu, 20 Apr 2017 04:06:13 -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=BPWANFps7kxGE/663G/x+6asiqZ4XJ3o6dSROX2FEAE=; b=b3o+aP1cS0RGeVs3Zh1WVqphm120vCbPDm84QgkAP+ihlmRyfBBit8gl/KxqmCYI2f UjB7nY7ENepmZv8p7rVg9JkzZBHViaK2Ay3UIFdz9IseqUJBPKc6yYyuv6tLpKaAB1EW T2ufpZIW2QlOC/ZliPqkYiU/reOW9c+oeC7f4hOVZSuqtU2oES2hKpbobBmcwciAg/Iv BDC/firV+ySVDU8VY9SPd93VMHnBVWQSCzLHi9II2OfudDw4KB0L49XBC/naKaN4zoGv l60yDEcVjzfVHJK6F6+4B1x7vDbbO4geYtVRBJTDvYtckifqBr1GgD6kyVep5m5yyC6F pylQ== 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=BPWANFps7kxGE/663G/x+6asiqZ4XJ3o6dSROX2FEAE=; b=JNhhrYhmJSQI8CqvXQ4fkwd6uzKcATOE/yo7XRODvBfoIBzYAqSbUi4F5r/p7u8OqM Q2uUzfW2jogTGdjI3jDnpBiyDxYfUc/bx1EbnXG3caUHswmfttM1Oi5cWQCBjg03UQNy 1NM9zKJbuCt66/okhFsS4ZpHT5spE5XoGgs3VZRGxBR0qu41Aqcb9NSjDEytYOT2Ek19 xaeHY8fV10ja+1+JsFGhaXINZqmEwrccW5nI2O38ijDyLdJGnkhL6t1egOJBr+JBLGF+ eBPhdMER4USkzM6iRvIMSV/Y+c8C4nwyM0duiRJzRztuL/jBqEnjT33aAxvXx7RUKCX4 LwxQ== X-Gm-Message-State: AN3rC/6FiTDXiBXP2JuS36uKgrpV4IxbZtjp4dDIVH2C/BhQu/i94T1A 3NFYN+Lhougq9Wr+QqHoMTvlDB2Tj8zU X-Received: by 10.157.11.123 with SMTP id p56mr3590104otd.149.1492686372666; Thu, 20 Apr 2017 04:06:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.18.200 with HTTP; Thu, 20 Apr 2017 04:06:12 -0700 (PDT) In-Reply-To: <87lgqwa4tg.fsf@drapion.f-secure.com> 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:06:12 +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 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. >> 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 * events can be destroyed now. --- -- 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..5b14890 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -295,6 +295,17 @@ 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/mask 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). + */ + kevent->mask = 0; + ret = 0; + } + /* * Permission events get queued to wait for response. Other