Page MenuHomeFreeBSD

makefs: do not call brelse if bread returns an error
ClosedPublic

Authored by emaste on Mar 13 2023, 9:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
May 11 2024, 9:50 AM
Unknown Object (File)
May 6 2024, 1:41 AM
Unknown Object (File)
May 6 2024, 1:41 AM
Unknown Object (File)
May 6 2024, 1:41 AM
Unknown Object (File)
May 6 2024, 1:29 AM
Unknown Object (File)
Feb 19 2024, 10:11 PM
Unknown Object (File)
Feb 10 2024, 9:39 PM
Unknown Object (File)
Dec 20 2023, 3:35 AM
Subscribers
None

Details

Summary
If bread returns an error there is no bp to brelse.  One of these
changes was taken from NetBSD commit 0a62dad69f62 ("This works well
enough to populate..."), the rest were found by looking for the same
pattern.

Sponsored by:   The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste created this revision.
emaste added inline comments.
usr.sbin/makefs/ffs/ffs_alloc.c
308

this is the one that came from NetBSD

imp accepted this revision.EditedMar 13 2023, 9:09 PM

I wonder if we can use a https://github.com/coccinelle/coccinelle script to find all of these :)

bread can't return an error, though. all paths go through return (0) or err(1.,,,)
and bp is allocated unconditionally (with a call to exit it if the malloc fails). So if
we did start returning errors for bread, we'd have to make sure we brelse there.

This revision is now accepted and ready to land.Mar 13 2023, 9:09 PM

So if we did start returning errors for bread, we'd have to make sure we brelse there.

Yeah; kernel bread does:

/*
 * Attempt to initiate asynchronous I/O on read-ahead blocks.
 */
breada(vp, rablkno, rabsize, cnt, cred, flags, ckhashfunc);

rv = 0;
if (readwait) {
        rv = bufwait(bp);
        if (rv != 0) {
                brelse(bp);
                *bpp = NULL;
        }
}
return (rv);

so if we're going to start returning errors from our bread we'll want to do the internal brelse as well.

For reference, NetBSD commit is:

commit cb2aeaa9b62f6985be27797724e2f66c74dca670
Author: christos <christos@NetBSD.org>
Date:   Mon Mar 13 22:17:24 2023 +0000

    Don't brelse() if bread() fails. The kernel does this for us. Our bread()
    implementation just exits on failure, but if it didn't we would double-free.
    From Ed Maste (https://reviews.freebsd.org/D39069)