Re: CVS commit: src/sys/ufs/ufs
On Sun, Feb 24, 2019 at 10:13:40PM +0100, Tobias Nygren wrote: > On Sun, 24 Feb 2019 19:06:40 + > Michael van Elst wrote: > > > To generate a diff of this commit: > > cvs rdiff -u -r1.242 -r1.243 src/sys/ufs/ufs/ufs_vnops.c > > > + rawbuf -= dropend; > > I guess this should be rawbufmax, not rawbuf. Yes, already fixed. -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
re: CVS commit: src/sys/netinet
> If you think that this is better and it works, please go for it. please, no. don't duplicate prototypes in a way that changes won't be detected. ie, if the real sctp code changes, today the compat caller will fail to build until updated. with this change, it will build and be entirely wrong. it is almost always wrong to duplicate prototypes. .mrg.
Re: CVS commit: src/sys/ufs/ufs
On Mon, Feb 25, 2019 at 06:07:23AM +, David Holland wrote:
> that one doesn't set dropend correctly for small buffers, outsmarted
> myself while writing it.
Better change (still against 1.242) that makes the logic much
simpler. Will test this overnight...
Index: ufs_vnops.c
===
RCS file: /cvsroot/src/sys/ufs/ufs/ufs_vnops.c,v
retrieving revision 1.239
diff -u -p -r1.239 ufs_vnops.c
--- ufs_vnops.c 28 Oct 2017 00:37:13 - 1.239
+++ ufs_vnops.c 25 Feb 2019 06:58:52 -
@@ -1233,7 +1233,7 @@ ufs_readdir(void *v)
#endif
/* caller's buffer */
struct uio *calleruio = ap->a_uio;
- off_t startoffset, endoffset;
+ off_t startoffset;
size_t callerbytes;
off_t curoffset;
/* dirent production buffer */
@@ -1244,8 +1244,8 @@ ufs_readdir(void *v)
off_t *cookies;
size_t numcookies, maxcookies;
/* disk buffer */
- off_t physstart, physend;
- size_t skipstart, dropend;
+ off_t physstart;
+ size_t skipstart;
char*rawbuf;
size_t rawbufmax, rawbytes;
struct uio rawuio;
@@ -1256,29 +1256,60 @@ ufs_readdir(void *v)
KASSERT(VOP_ISLOCKED(vp));
- /* figure out where we want to read */
+#define UFS_READDIR_ARBITRARY_MAX (1024*1024)
callerbytes = calleruio->uio_resid;
- startoffset = calleruio->uio_offset;
- endoffset = startoffset + callerbytes;
-
if (callerbytes < _DIRENT_MINSIZE(dirent)) {
/* no room for even one struct dirent */
return EINVAL;
}
+ if (callerbytes > UFS_READDIR_ARBITRARY_MAX) {
+ callerbytes = UFS_READDIR_ARBITRARY_MAX;
+ }
- /* round start and end down to block boundaries */
+ /*
+* Figure out where to start reading. Round the start down to
+* a block boundary: we need to start at the beginning of a
+* block in order to read the directory correctly.
+*
+* skipstart is the number of bytes we need to read in
+* (because we need to start at the beginning of a block) but
+* not transfer to the user.
+*/
+ startoffset = calleruio->uio_offset;
physstart = startoffset & ~(off_t)(ump->um_dirblksiz - 1);
- physend = endoffset & ~(off_t)(ump->um_dirblksiz - 1);
skipstart = startoffset - physstart;
- dropend = endoffset - physend;
- if (callerbytes - dropend < _DIRENT_MINSIZE(rawdp)) {
- /* no room for even one struct direct */
- return EINVAL;
- }
+ /*
+* Now figure out how much to read.
+*
+* callerbytes tells us how much data we can send back to the
+* user. Assume as a starting point that we want to read
+* roughly the same volume of struct directs from disk as the
+* volume of struct dirents we want to send to the user; this
+* is not super accurate for large numbers of small names but
+* will serve well enough. It's ok to underpopulate the user's
+* buffer, and most directories are only a couple blocks
+* anyway.
+*
+* However much we decide we want, stop on a block boundary.
+* That way the copying code below doesn't need to worry about
+* finding partial entries in the transfer buffer.
+*
+* Do this by rounding down (not up) by default as that will
+* with luck avoid needing to scan the next block twice; but
+* always read in at least one block.
+*/
- /* how much to actually read */
- rawbufmax = callerbytes + skipstart - dropend;
+ /* Base buffer space for CALLERBYTES of new data */
+ rawbufmax = callerbytes + skipstart;
+
+ /* Round down to an integral number of blocks */
+ rawbufmax &= ~(off_t)(ump->um_dirblksiz - 1);
+
+ /* but always at least one block */
+ if (rawbufmax == 0) {
+ rawbufmax = ump->um_dirblksiz;
+ }
/* read it */
rawbuf = kmem_alloc(rawbufmax, KM_SLEEP);
--
David A. Holland
dholl...@netbsd.org
Re: CVS commit: src/sys/netinet
On 24.02.2019 23:55, Robert Swindells wrote: > > Kamil Rytarowski wrote: >> Module Name:src >> Committed By: kamil >> Date: Sun Feb 24 17:01:52 UTC 2019 >> >> Modified Files: >>src/sys/netinet: sctp_asconf.h >> >> Log Message: >> Appease GCC7 in sctp_asconf.h >> >> Do not declare types inside function parameter list. >> Add decklarations of types before these function prototypes. > > My local fix for this was to remove sctp_asconf.h from compat_stub.c and > just add prototypes for the two functions needed. This reduces the diffs > to the previous rtsock.c. > > I don't think sctp_asconf.h is supposed to be used outside of network > sources. > If you think that this is better and it works, please go for it. > Index: compat_stub.c > === > RCS file: /cvsroot/src/sys/kern/compat_stub.c,v > retrieving revision 1.8 > diff -u -r1.8 compat_stub.c > --- compat_stub.c 5 Feb 2019 23:28:02 - 1.8 > +++ compat_stub.c 24 Feb 2019 22:51:12 - > @@ -44,10 +44,6 @@ > #include > #endif > > -#ifdef SCTP > -#include > -#endif > - > /* > * Routine vectors for compat_50___sys_ntp_gettime > * > @@ -71,6 +67,9 @@ > */ > > #ifdef SCTP > +extern void sctp_add_ip_address(struct ifaddr *); > +extern void sctp_delete_ip_address(struct ifaddr *); > + > void (*vec_sctp_add_ip_address)(struct ifaddr *) = sctp_add_ip_address; > void (*vec_sctp_delete_ip_address)(struct ifaddr *) = sctp_delete_ip_address; > #else > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/ufs/ufs
On Mon, Feb 25, 2019 at 05:50:08AM +, David Holland wrote: > Furthermore, this: > > > + rawbuf -= dropend; > > is entirely wrong (it needs to be "rawbufmax") and without that bound > on rawbufmax the code is unsafe... I repaired this bit just now, so it's not an overt hazard any more. I still don't like this change all that much but whatever, I suppose... > Here's the fix I got bogged down trying to build and test, which also > adds a missing upper bound on callerbytes: that one doesn't set dropend correctly for small buffers, outsmarted myself while writing it. -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/ufs/ufs
On Sun, Feb 24, 2019 at 07:51:24PM -0500, Christos Zoulas wrote: > Module Name: src > Committed By:christos > Date:Mon Feb 25 00:51:24 UTC 2019 > > Modified Files: > src/sys/ufs/ufs: ufs_vnops.c > > Log Message: > drop unused dropping this logic is wrong... -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/netinet
Kamil Rytarowski wrote: >Module Name:src >Committed By: kamil >Date: Sun Feb 24 17:01:52 UTC 2019 > >Modified Files: >src/sys/netinet: sctp_asconf.h > >Log Message: >Appease GCC7 in sctp_asconf.h > >Do not declare types inside function parameter list. >Add decklarations of types before these function prototypes. My local fix for this was to remove sctp_asconf.h from compat_stub.c and just add prototypes for the two functions needed. This reduces the diffs to the previous rtsock.c. I don't think sctp_asconf.h is supposed to be used outside of network sources. Index: compat_stub.c === RCS file: /cvsroot/src/sys/kern/compat_stub.c,v retrieving revision 1.8 diff -u -r1.8 compat_stub.c --- compat_stub.c 5 Feb 2019 23:28:02 - 1.8 +++ compat_stub.c 24 Feb 2019 22:51:12 - @@ -44,10 +44,6 @@ #include #endif -#ifdef SCTP -#include -#endif - /* * Routine vectors for compat_50___sys_ntp_gettime * @@ -71,6 +67,9 @@ */ #ifdef SCTP +extern void sctp_add_ip_address(struct ifaddr *); +extern void sctp_delete_ip_address(struct ifaddr *); + void (*vec_sctp_add_ip_address)(struct ifaddr *) = sctp_add_ip_address; void (*vec_sctp_delete_ip_address)(struct ifaddr *) = sctp_delete_ip_address; #else
Re: CVS commit: src/sys/ufs/ufs
On Sun, 24 Feb 2019 19:06:40 + Michael van Elst wrote: > To generate a diff of this commit: > cvs rdiff -u -r1.242 -r1.243 src/sys/ufs/ufs/ufs_vnops.c > + rawbuf -= dropend; I guess this should be rawbufmax, not rawbuf.
Re: CVS commit: src/sys/netinet
On Sun, Feb 24, 2019 at 09:43:52PM +0100, Kamil Rytarowski wrote: > I consider that this is just GCC specific behavior to make some warnings > fatal depending on driver configuration. This is typical behavior of GCC > that we keep observing all over again. No, this is very different to optimizer and target specific warnings. There must be a fundamental difference in includes, it should all be pretty obvious from comparing .depend / *.d files. IMO this should be analyzed clearly instead of papered over. Martin
Re: CVS commit: src/sys/netinet
On 24.02.2019 21:38, Martin Husemann wrote: > On Sun, Feb 24, 2019 at 09:36:55PM +0100, Kamil Rytarowski wrote: >> My only specific change was NetBSD/i386 kernel=GENERIC with kUBSan and >> KCOV enabled. > > This does not answer the question. What does enabling kUBSan/KCOV break > to make this error show up in your compilation, but not in our default > builds? > > Martin > I consider that this is just GCC specific behavior to make some warnings fatal depending on driver configuration. This is typical behavior of GCC that we keep observing all over again. Exact answer of GCC behavior exceeds my capabilities as of now (mostly no time). signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/netinet
On 24.02.2019 21:43, Kamil Rytarowski wrote: > On 24.02.2019 21:38, Martin Husemann wrote: >> On Sun, Feb 24, 2019 at 09:36:55PM +0100, Kamil Rytarowski wrote: >>> My only specific change was NetBSD/i386 kernel=GENERIC with kUBSan and >>> KCOV enabled. >> >> This does not answer the question. What does enabling kUBSan/KCOV break >> to make this error show up in your compilation, but not in our default >> builds? >> >> Martin >> > > I consider that this is just GCC specific behavior to make some warnings > fatal depending on driver configuration. This is typical behavior of GCC > that we keep observing all over again. > > Exact answer of GCC behavior exceeds my capabilities as of now (mostly > no time). > From what is important. I'm going to enable sanitized kernel builds to catch these issues quickly. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/netinet
On Sun, Feb 24, 2019 at 09:36:55PM +0100, Kamil Rytarowski wrote: > My only specific change was NetBSD/i386 kernel=GENERIC with kUBSan and > KCOV enabled. This does not answer the question. What does enabling kUBSan/KCOV break to make this error show up in your compilation, but not in our default builds? Martin
Re: CVS commit: src/sys/netinet
On 24.02.2019 20:39, Martin Husemann wrote: > On Sun, Feb 24, 2019 at 07:20:10PM +0100, Kamil Rytarowski wrote: >> On 24.02.2019 19:15, David Holland wrote: >>> On Sun, Feb 24, 2019 at 05:01:52PM +, Kamil Rytarowski wrote: >>> > Modified Files: >>> > src/sys/netinet: sctp_asconf.h >>> > >>> > Log Message: >>> > Appease GCC7 in sctp_asconf.h >>> > >>> > Do not declare types inside function parameter list. >>> > Add decklarations of types before these function prototypes. >>> >>> That warning isn't new... are you sure the observed behavior isn't a >>> symptom of some other problem elsewhere? :-/ >>> >> >> >> http://netbsd.org/~kamil/patch-00090-sctp_asconf.h.txt > > The question is: why does this show up in *your* build but nowhere else? > > Martin > > > My only specific change was NetBSD/i386 kernel=GENERIC with kUBSan and KCOV enabled. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/ufs/ufs
On Sun, Feb 24, 2019 at 07:41:59PM +, m...@netbsd.org wrote: > something like the overflow is undefined behaviour, so it cannot > happen, so the branch checking that it happened is eliminated. *Signed* integer overflow is undefined behavior. *Unsigned* integer overflow is well defined. Joerg
Re: CVS commit: src/sys/ufs/ufs
On Sun, Feb 24, 2019 at 07:41:59PM +, m...@netbsd.org wrote: > something like the overflow is undefined behaviour, so it cannot > happen, so the branch checking that it happened is eliminated. Integer overflow is not undefined, but implemenation defined behaviour. Martin
Re: CVS commit: src/sys/ufs/ufs
On Sun, Feb 24, 2019 at 07:06:40PM +, Michael van Elst wrote: > While here, also check for arithmetic overflow. > + /* how much to actually read */ > + rawbufmax = callerbytes + skipstart; > + if (rawbufmax < callerbytes) > + return EINVAL; hmm, I"m under the impression that checking for overflow without upsetting the compiler is a delicate matter. something like the overflow is undefined behaviour, so it cannot happen, so the branch checking that it happened is eliminated.
Re: CVS commit: src/sys/netinet
On Sun, Feb 24, 2019 at 07:20:10PM +0100, Kamil Rytarowski wrote: > On 24.02.2019 19:15, David Holland wrote: > > On Sun, Feb 24, 2019 at 05:01:52PM +, Kamil Rytarowski wrote: > > > Modified Files: > > > src/sys/netinet: sctp_asconf.h > > > > > > Log Message: > > > Appease GCC7 in sctp_asconf.h > > > > > > Do not declare types inside function parameter list. > > > Add decklarations of types before these function prototypes. > > > > That warning isn't new... are you sure the observed behavior isn't a > > symptom of some other problem elsewhere? :-/ > > > > > http://netbsd.org/~kamil/patch-00090-sctp_asconf.h.txt The question is: why does this show up in *your* build but nowhere else? Martin
Re: CVS commit: src/sys/netinet
On 24.02.2019 19:15, David Holland wrote: > On Sun, Feb 24, 2019 at 05:01:52PM +, Kamil Rytarowski wrote: > > Modified Files: > >src/sys/netinet: sctp_asconf.h > > > > Log Message: > > Appease GCC7 in sctp_asconf.h > > > > Do not declare types inside function parameter list. > > Add decklarations of types before these function prototypes. > > That warning isn't new... are you sure the observed behavior isn't a > symptom of some other problem elsewhere? :-/ > http://netbsd.org/~kamil/patch-00090-sctp_asconf.h.txt signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/netinet
On Sun, Feb 24, 2019 at 05:01:52PM +, Kamil Rytarowski wrote: > Modified Files: > src/sys/netinet: sctp_asconf.h > > Log Message: > Appease GCC7 in sctp_asconf.h > > Do not declare types inside function parameter list. > Add decklarations of types before these function prototypes. That warning isn't new... are you sure the observed behavior isn't a symptom of some other problem elsewhere? :-/ -- David A. Holland dholl...@netbsd.org
