NetBSD Problem Report #51418

From www@NetBSD.org  Wed Aug 17 11:36:32 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id D3CDA7A266
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 17 Aug 2016 11:36:32 +0000 (UTC)
Message-Id: <20160817113631.729737A2D5@mollari.NetBSD.org>
Date: Wed, 17 Aug 2016 11:36:31 +0000 (UTC)
From: joseyluis@gmail.com
Reply-To: joseyluis@gmail.com
To: gnats-bugs@NetBSD.org
Subject: Fix incore src/sbin/fsck_lfs/bufcache.c
X-Send-Pr-Version: www-1.0

>Number:         51418
>Category:       bin
>Synopsis:       Fix incore src/sbin/fsck_lfs/bufcache.c
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    dholland
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Aug 17 11:40:00 +0000 2016
>Closed-Date:    Sat Jun 09 21:38:56 +0000 2018
>Last-Modified:  Thu Jun 14 19:40:01 +0000 2018
>Originator:     Jose Luis Rodriguez Garcia
>Release:        current and previous releases
>Organization:
>Environment:
>Description:
incore function from buffercache.c uses int instead of daddr_t
 it must be: incore(struct uvnode * vp, int lbn)
instead of: incore(struct uvnode * vp, daddr_t lbn)

It produces that daddr_t (that is unint64_t) is casted as int.
In 32 bits platforms (i386 at last), it produces that adress > 1 Tbyte (assuming a 512 DEV_BSIZE), are converted to a negative number. It produces the next problems in 32 bits architectures:

1- Blocks higher than 1 Tbyte aren't cached in the buffer cache. Its buffers aren't found by the incore function, because the real disk adress is compared against a negative number
2- A unique block is cached in several buffers, becuase incore never founds the block. getblock funtion wastes memory adding buffers that won't be found.
3- Potential corruption: A block marked with B_DELWRI (dirty cache) can be contained in different buffers. What buffer will be written first?

I think that the fix must be pulled to previous releases.
>How-To-Repeat:
code review
>Fix:
I have tested the compilation of the fix with a ./build.sh build

Index: bufcache.c
===================================================================
RCS file: /cvsroot/src/sbin/fsck_lfs/bufcache.c,v
retrieving revision 1.16
diff -u -r1.16 bufcache.c
--- bufcache.c  31 Jul 2016 18:27:26 -0000      1.16
+++ bufcache.c  17 Aug 2016 11:26:49 -0000
@@ -194,7 +194,7 @@

 /* Return a buffer if it is in the cache, otherwise return NULL. */
 struct ubuf *
-incore(struct uvnode * vp, int lbn)
+incore(struct uvnode * vp, daddr_t lbn)
 {
        struct ubuf *bp;
        int hash, depth;
Index: bufcache.h
===================================================================
RCS file: /cvsroot/src/sbin/fsck_lfs/bufcache.h,v
retrieving revision 1.12
diff -u -r1.12 bufcache.h
--- bufcache.h  29 Mar 2015 19:35:58 -0000      1.12
+++ bufcache.h  17 Aug 2016 11:26:49 -0000
@@ -116,7 +116,7 @@
 void bufstats(void);
 void buf_destroy(struct ubuf *);
 void bremfree(struct ubuf *);
-struct ubuf *incore(struct uvnode *, int);
+struct ubuf *incore(struct uvnode *, daddr_t);
 struct ubuf *getblk(struct uvnode *, daddr_t, int);
 void bwrite(struct ubuf *);
 void brelse(struct ubuf *, int);

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: bin-bug-people->dholland
Responsible-Changed-By: maya@NetBSD.org
Responsible-Changed-When: Sat, 03 Dec 2016 11:20:32 +0000
Responsible-Changed-Why:
more lfs


From: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c
Date: Mon, 5 Dec 2016 15:05:27 +0100

 Looking in other pieces of code, it seems that sometimes negative
 numbers are used. I did some tests time ago, and this patch broke
 somethings.

 The treatment of disk address seems inconsistent. In buffcache.c uses
 mainly daddr_t (unsigned) and in other parts of the code uses signed
 numbers. It seems confussing and prone to errors.

 On Wed, Aug 17, 2016 at 1:40 PM,  <gnats-admin@netbsd.org> wrote:
 > Thank you very much for your problem report.
 > It has the internal identification `bin/51418'.
 > The individual assigned to look at your
 > report is: bin-bug-people.
 >
 >>Category:       bin
 >>Responsible:    bin-bug-people
 >>Synopsis:       Fix incore src/sbin/fsck_lfs/bufcache.c
 >>Arrival-Date:   Wed Aug 17 11:40:00 +0000 2016
 >

From: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c
Date: Wed, 22 Nov 2017 12:39:29 +0100

 I have tested this patch one more time and it seems fine. Please commit it.

 I couldn't reproduce the previous failures, they must have been problems
 with my old QEMU/FreeBSD setup that it isn't enough stable.

 I have passed the fs tests, newfs, uncompress several tars and all has
 been ok. (doing more operations with the filesystem fails after/before
 the patch but these are other bugs of lfs).

 This patch is needed because on 32 bits systems (daddr_t is a int64_t)
 there will be corruption/slow performance for LFS fileystems > 1 Tbyte
 (LFS2 allows up to 2 Tbytes according to the roadmap) and LFS64.

 On Mon, Dec 5, 2016 at 3:10 PM, Jose Luis Rodriguez Garcia
 <joseyluis@gmail.com> wrote:
 > The following reply was made to PR bin/51418; it has been noted by GNATS.
 >
 > From: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
 > To: gnats-bugs@netbsd.org
 > Cc:
 > Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c
 > Date: Mon, 5 Dec 2016 15:05:27 +0100
 >
 >  Looking in other pieces of code, it seems that sometimes negative
 >  numbers are used. I did some tests time ago, and this patch broke
 >  somethings.
 >
 >  The treatment of disk address seems inconsistent. In buffcache.c uses
 >  mainly daddr_t (unsigned) and in other parts of the code uses signed
 >  numbers. It seems confussing and prone to errors.
 >
 >  On Wed, Aug 17, 2016 at 1:40 PM,  <gnats-admin@netbsd.org> wrote:
 >  > Thank you very much for your problem report.
 >  > It has the internal identification `bin/51418'.
 >  > The individual assigned to look at your
 >  > report is: bin-bug-people.
 >  >
 >  >>Category:       bin
 >  >>Responsible:    bin-bug-people
 >  >>Synopsis:       Fix incore src/sbin/fsck_lfs/bufcache.c
 >  >>Arrival-Date:   Wed Aug 17 11:40:00 +0000 2016
 >  >
 >

From: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
To: gnats-bugs@netbsd.org
Cc: David Holland <dholland@netbsd.org>, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c
Date: Fri, 30 Mar 2018 04:14:37 +0200

 Could the proposed patch in the original bug report patch be?

 It is a simple fix that avoids some corruptions in big LFS volumes.

 Best Regards

From: David Holland <dholland@netbsd.org>
To: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
Cc: gnats-bugs@netbsd.org
Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c
Date: Fri, 30 Mar 2018 06:50:12 +0000

 On Fri, Mar 30, 2018 at 04:14:37AM +0200, Jose Luis Rodriguez Garcia wrote:
  > Could the proposed patch in the original bug report patch be?
  > 
  > It is a simple fix that avoids some corruptions in big LFS volumes.

 sorry, haven't had time to think about it or much of anything else in
 the past 2-3 months :(

 -- 
 David A. Holland
 dholland@netbsd.org

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51418 CVS commit: src/sbin/fsck_lfs
Date: Fri, 30 Mar 2018 08:56:46 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Fri Mar 30 12:56:46 UTC 2018

 Modified Files:
 	src/sbin/fsck_lfs: bufcache.c bufcache.h

 Log Message:
 PR/51418: Jose Luis Rodriguez Garcia: Fix incore src/sbin/fsck_lfs/bufcache.c
 XXX: pullup-8, pullup-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.19 -r1.20 src/sbin/fsck_lfs/bufcache.c
 cvs rdiff -u -r1.13 -r1.14 src/sbin/fsck_lfs/bufcache.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, dholland@NetBSD.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org, joseyluis@gmail.com
Cc: 
Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c
Date: Fri, 30 Mar 2018 08:57:06 -0400

 On Mar 30,  2:15am, joseyluis@gmail.com (Jose Luis Rodriguez Garcia) wrote:
 -- Subject: Re: bin/51418: Fix incore src/sbin/fsck_lfs/bufcache.c

 |  It is a simple fix that avoids some corruptions in big LFS volumes.

 Indeed, thanks for your patience!

 christos

State-Changed-From-To: open->pending-pullups
State-Changed-By: maya@NetBSD.org
State-Changed-When: Mon, 04 Jun 2018 08:58:36 +0000
State-Changed-Why:
pullup-8 #855, pullup-7 #1612


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51418 CVS commit: [netbsd-7] src/sbin/fsck_lfs
Date: Wed, 6 Jun 2018 15:38:05 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Wed Jun  6 15:38:05 UTC 2018

 Modified Files:
 	src/sbin/fsck_lfs [netbsd-7]: bufcache.c bufcache.h

 Log Message:
 Pull up following revision(s) (requested by maya in ticket #1612):

 	sbin/fsck_lfs/bufcache.h: revision 1.14
 	sbin/fsck_lfs/bufcache.c: revision 1.20

 PR/51418: Jose Luis Rodriguez Garcia: Fix incore src/sbin/fsck_lfs/bufcache.c
 XXX: pullup-8, pullup-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.14 -r1.14.4.1 src/sbin/fsck_lfs/bufcache.c
 cvs rdiff -u -r1.11 -r1.11.38.1 src/sbin/fsck_lfs/bufcache.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51418 CVS commit: [netbsd-8] src/sbin/fsck_lfs
Date: Fri, 8 Jun 2018 10:21:12 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Fri Jun  8 10:21:12 UTC 2018

 Modified Files:
 	src/sbin/fsck_lfs [netbsd-8]: bufcache.c bufcache.h

 Log Message:
 Pull up following revision(s) (requested by maya in ticket #855):

 	sbin/fsck_lfs/bufcache.h: revision 1.14
 	sbin/fsck_lfs/bufcache.c: revision 1.20

 PR/51418: Jose Luis Rodriguez Garcia: Fix incore src/sbin/fsck_lfs/bufcache.c
 XXX: pullup-8, pullup-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.19 -r1.19.6.1 src/sbin/fsck_lfs/bufcache.c
 cvs rdiff -u -r1.13 -r1.13.6.1 src/sbin/fsck_lfs/bufcache.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: maya@NetBSD.org
State-Changed-When: Sat, 09 Jun 2018 21:38:56 +0000
State-Changed-Why:
Pullups completed, thanks for the patch!


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51418 CVS commit: [netbsd-7-1] src/sbin/fsck_lfs
Date: Thu, 14 Jun 2018 19:36:53 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Jun 14 19:36:53 UTC 2018

 Modified Files:
 	src/sbin/fsck_lfs [netbsd-7-1]: bufcache.c bufcache.h

 Log Message:
 Pull up following revision(s) (requested by maya in ticket #1612):

 	sbin/fsck_lfs/bufcache.h: revision 1.14
 	sbin/fsck_lfs/bufcache.c: revision 1.20

 PR/51418: Jose Luis Rodriguez Garcia: Fix incore src/sbin/fsck_lfs/bufcache.c
 XXX: pullup-8, pullup-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.14 -r1.14.10.1 src/sbin/fsck_lfs/bufcache.c
 cvs rdiff -u -r1.11 -r1.11.44.1 src/sbin/fsck_lfs/bufcache.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51418 CVS commit: [netbsd-7-0] src/sbin/fsck_lfs
Date: Thu, 14 Jun 2018 19:37:56 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Jun 14 19:37:56 UTC 2018

 Modified Files:
 	src/sbin/fsck_lfs [netbsd-7-0]: bufcache.c bufcache.h

 Log Message:
 Pull up following revision(s) (requested by maya in ticket #1612):

 	sbin/fsck_lfs/bufcache.h: revision 1.14
 	sbin/fsck_lfs/bufcache.c: revision 1.20

 PR/51418: Jose Luis Rodriguez Garcia: Fix incore src/sbin/fsck_lfs/bufcache.c
 XXX: pullup-8, pullup-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.14 -r1.14.6.1 src/sbin/fsck_lfs/bufcache.c
 cvs rdiff -u -r1.11 -r1.11.40.1 src/sbin/fsck_lfs/bufcache.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search