Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230704092309.22669-1-vegard.nossum@oracle.com>
 <20230704100852.23452-1-vegard.nossum@oracle.com> <CAHk-=wgy2q9viwq-bAqaq8X_WQHNqDsu1AzfeYXAcDQRZmCoAw@mail.gmail.com>
 <7cf14d1b-1a3a-fea7-0d2c-409ab828569b@oracle.com>
In-Reply-To: <7cf14d1b-1a3a-fea7-0d2c-409ab828569b@oracle.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 4 Jul 2023 10:10:14 -0700
Message-ID: <CAHk-=wg70vQAKPOqo-Ym6n47KSpm09kc-dmKME9Sd1HUcdZ-sg@mail.gmail.com>
Subject: Re: [PATCH] module: always complete idempotent loads
To:     Vegard Nossum <vegard.nossum@oracle.com>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        George Kennedy <george.kennedy@oracle.com>,
        linux-kernel@vger.kernel.org, linux-modules@vger.kernel.org,
        Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>,
        syzbot+9c2bdc9d24e4a7abe741@syzkaller.appspotmail.com,
        Johan Hovold <johan@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Rudi Heitbaum <rudi@heitbaum.com>,
        David Hildenbrand <david@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 4 Jul 2023 at 08:12, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>
> Maybe just do the f_mode check in finit_module()? Or... new helper,
> fdget_mode()??

I actually wanted to do a fdget_read/write() helper long ago: we
already basically pass in a "this mode is not ok" flag for the
FMODE_PATH case, and it's sad how many extra "do we have
FMODE_READ/WRITE" tests we have when it would actually make tons of
sense to just check it at fdget() time.

But I created the patch, and then decided it was just churn for
something that didn't matter a lot.

The existing "mode" argument to __fget_light() and __fget() would end
up split into "error if these bits are set" (existing) and "error if
these bits are _not_ set" (new) arguments.

It was straightforward, but I looked at the patch, and then looked at
all the existing users that currently check the error separately, and
went "bah", and threw it all away.

Which is not to say it's not the right thing to do. Maybe we should at
some point. But doing it right (ie doing it in that helper before we
even bother with reference counting etc) is just a bit too much work.

The alternative is, of course, to just have a *truly* stupid wrapper
that does the error checking and does the "fdput()" if FMODE_READ
wasn't set. But that's just disgusting.

Anyway, for this module case, I just moved it out to the caller.

> Apart from this, there is another bit that looks a bit weird:
>
> len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
> if (len < 0) {
>      mod_stat_inc(&failed_kreads);
>      mod_stat_add_long(len, &invalid_kread_bytes);
>
> I don't think we should be adding error codes to byte counts.

Ack. I fixed that at the same time as a "multiple problems with the
error paths".

                 Linus