Re: svn commit: r232071 - head/sys/vm
On Mon, May 27, 2019 at 02:34:09PM -0700, Rodney W. Grimes wrote: > I do not at all mean to discourage what you are doing, it is good to go > over static analysis reports, the problem is that there are often many > false positives, There are tons of them indeed, but that's normal for any static analysis tool. I've attempted to clean up the original log by ignoring the most noisy and dubious diagnostics and maintaining false positives list, starting with the ones pointed out by kib@. > It would be nice if we had a "team" that looked at all the Coverity > data, and any other data like what you have offered up here. Part of > the problem is that few want to do that work, or those that do want to > think it is low hanging fruit that anyone can do. That's exactly right, generating the check log is easy, finding people motivated enough to triage it (or finding the motivation within oneself) is tough. Maybe this ~2.4k line log would attract more interest than the first raw, full >16k line one: freefall:/home/danfe/pvs-kernel-filtered-2019-05-28.log.xz I'd appreciate if experts in their areas (cam/scsi, dev drivers, geom, kern, netinet, nfs, etc.) glanced over and see if they might help with anything there. If you find a false positive, please tell me so I can add it to the ignore list. Thanks! ./danfe ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
> On Mon, 2019-05-27 at 14:34 -0700, Rodney W. Grimes wrote: > > I just ask that before a change be made that starts from some > > static analysis tool that a formal code review occur before > > the change is committed. > > In a word: No. > > We're not talking about some inexperienced coder committing dumb fixes > to silence warnings here. In this specific instance perhaps not so, but unless I am miss remebering the thread either a change was suggested that came from a static analysis tool that an area expert vetoed. That is ineffect what I advocated, so you can say No, but that is not what actually occurred. Furthermore we have had "inexperienced coder's" commit dumb fixes to silence a wwarning that later had to be reverted and cleaned up properly. So again, you can say No, but your basis is wrong. > What I've seen so far is that fixes are > being committed by the people who originally wrote or currently > maintain the code in question. Nothing bad is happening, so nothing > about the process needs to be fixed or changed in any way. This is not what is going on. > Process for process' sake is just obstruction. This is not process for process sake, this is process to stop bad code changes, and you can not say it is not happening, because it is. > -- Ian -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
On Mon, 2019-05-27 at 14:34 -0700, Rodney W. Grimes wrote: > I just ask that before a change be made that starts from some > static analysis tool that a formal code review occur before > the change is committed. In a word: No. We're not talking about some inexperienced coder committing dumb fixes to silence warnings here. What I've seen so far is that fixes are being committed by the people who originally wrote or currently maintain the code in question. Nothing bad is happening, so nothing about the process needs to be fixed or changed in any way. Process for process' sake is just obstruction. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
> On Mon, May 27, 2019 at 07:21:24AM -0700, Rodney W. Grimes wrote: > > I wish to assert again that all changes based on static > > analysis tools require a formal code review by at minimum: > > a) An expert in static analysis tools or a group of people > > selected who we consider language experts. > > b) An area expert from the area that is being affected > > > > We as a project look bad by not having this minimal > > set of code reviews in place for things that are coming > > from a source that is frought with introducing bad > > change. > > Perhaps I should've just uploaded the log to let anyone interested > have a look themselves. I've been only checking the kernel so far, > but plan to cover userland as well later. I do not at all mean to discourage what you are doing, it is good to go over static analysis reports, the problem is that there are often many false positives, and also subtle things that need extreme caution when making repairs. I just ask that before a change be made that starts from some static analysis tool that a formal code review occur before the change is committed. > freefall:/home/danfe/pvs-studio-2019-05-27.log.xz It would be nice if we had a "team" that looked at all the coverity data, and any other data like what you have offered up here. Part of the problem is that few want to do that work, or those that do want to think it is low hanging fruit that anyone can do. I would suggest the opposite, some times it might be low hanging fruit but often it is not, it needs expertise or mistakes are going to happen, and often those mistakes go un caught for some time. Recently a change was made based on a coverity report about not freeing something, some free's got added, one of our downsteam consumers caught this change on import and reported it as a multiple free situation, which was caused by pointer aliasing, something that most static analyzers are horrible at. This self induced bug was in tree for ~2 months and iirc made it in a release. I know I personally saw the original commit, and it looked ok to my un trained eyes, only upon the aliasing being pointed out did my light bulb go off that this was a mistake to have changed the code in the way it was changed. Can I get a hands up for team "Static Blue"? > ./danfe -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
On Mon, May 27, 2019 at 07:21:24AM -0700, Rodney W. Grimes wrote: > ... > We as a project look bad by not having this minimal set of code > reviews in place for things that are coming from a source that is > frought with introducing bad change. We do *not* look bad because none of those warnings and errors suggested by (any) static analysis tools were ever passed without all the due checks by either the original committer, the last committer who touched the files, or opening a PR in less straight-forward cases. I have several potential issues in my queue from the last run which I might submit for the further analysis (via email or PR), but for now since I've published the full log (very noisy), let's see if it gets any traction on its own. Some people had contacted me off-the-list and expressed their interest. I'll revisit it in a few weeks to see if nagging developers and opening PRs is still needed, or the bugs are being voluntarily squished by responsible (and having proper domain-knowledge) parties. ./danfe ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
On Mon, May 27, 2019 at 07:21:24AM -0700, Rodney W. Grimes wrote: > I wish to assert again that all changes based on static > analysis tools require a formal code review by at minimum: > a) An expert in static analysis tools or a group of people > selected who we consider language experts. > b) An area expert from the area that is being affected > > We as a project look bad by not having this minimal > set of code reviews in place for things that are coming > from a source that is frought with introducing bad > change. Perhaps I should've just uploaded the log to let anyone interested have a look themselves. I've been only checking the kernel so far, but plan to cover userland as well later. freefall:/home/danfe/pvs-studio-2019-05-27.log.xz ./danfe ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
On Mon, May 27, 2019 at 02:21:55PM +0300, Konstantin Belousov wrote: > On Mon, May 27, 2019 at 09:52:56AM +, Alexey Dokuchaev wrote: > > On Thu, Feb 23, 2012 at 09:07:16PM +, Konstantin Belousov wrote: > > > ... > > > + /* > > > + * Decrement the object's writemappings, by swapping the start > > > + * and end arguments for vnode_pager_update_writecount(). If > > > + * there was not a race with vnode reclaimation, then the > > > + * vnode's v_writecount is decremented. > > > + */ > > > + vnode_pager_update_writecount(object, end, start); > > > > ... here, first `end' is passed, then `start'. Is this intentional? > Did you read the comment right before the call ? I did, as I thought that explanation might be there, but it evaded me (also, I was subconsciously expecting it near the end of the comment), thanks for bring it to my attention and sorry for the noise. ./danfe ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
> On Mon, May 27, 2019 at 09:52:56AM +, Alexey Dokuchaev wrote:
> > On Thu, Feb 23, 2012 at 09:07:16PM +, Konstantin Belousov wrote:
> > > New Revision: 232071
> > > URL: http://svn.freebsd.org/changeset/base/232071
> > >
> > > Log:
> > > Account the writeable shared mappings backed by file in the vnode
> > > v_writecount. Keep the amount of the virtual address space used by
> > > the mappings in the new vm_object un_pager.vnp.writemappings
> > > counter. The vnode v_writecount is incremented when writemappings gets
> > > non-zero value, and decremented when writemappings is returned to
> > > zero.
> > >
> > > Writeable shared vnode-backed mappings are accounted for in vm_mmap(),
> > > and vm_map_insert() is instructed to set MAP_ENTRY_VN_WRITECNT flag on
> > > the created map entry. During deferred map entry deallocation,
> > > vm_map_process_deferred() checks for MAP_ENTRY_VN_WRITECOUNT and
> > > decrements writemappings for the vm object.
> > >
> > > Now, the writeable mount cannot be demoted to read-only while
> > > writeable shared mappings of the vnodes from the mount point
> > > exist. Also, execve(2) fails for such files with ETXTBUSY, as it
> > > should be.
> > >
> > > ...
> > > Modified: head/sys/vm/vnode_pager.c
> > > ==
> > > --- head/sys/vm/vnode_pager.c Thu Feb 23 20:58:52 2012
> > > (r232070)
> > > +++ head/sys/vm/vnode_pager.c Thu Feb 23 21:07:16 2012
> > > (r232071)
> > > @@ -1215,3 +1222,81 @@ vnode_pager_undirty_pages(vm_page_t *ma,
> > > }
> > > VM_OBJECT_UNLOCK(obj);
> > > }
> > > +
> > > +void
> > > +vnode_pager_update_writecount(vm_object_t object, vm_offset_t start,
> > > +vm_offset_t end)
> >
> > So, it is first `start, then `end', but below...
> >
> > > +{
> > > + struct vnode *vp;
> > > + vm_ooffset_t old_wm;
> > > +
> > > + ...
> > > +}
> > > +
> > > +void
> > > +vnode_pager_release_writecount(vm_object_t object, vm_offset_t start,
> > > +vm_offset_t end)
> > > +{
> > > + struct vnode *vp;
> > > + struct mount *mp;
> > > + vm_offset_t inc;
> > > + int vfslocked;
> > > +
> > > + VM_OBJECT_LOCK(object);
> > > +
> > > + /*
> > > + * First, recheck the object type to account for the race when
> > > + * the vnode is reclaimed.
> > > + */
> > > + if (object->type != OBJT_VNODE) {
> > > + VM_OBJECT_UNLOCK(object);
> > > + return;
> > > + }
> > > +
> > > + /*
> > > + * Optimize for the case when writemappings is not going to
> > > + * zero.
> > > + */
> > > + inc = end - start;
> > > + if (object->un_pager.vnp.writemappings != inc) {
> > > + object->un_pager.vnp.writemappings -= inc;
> > > + VM_OBJECT_UNLOCK(object);
> > > + return;
> > > + }
> > > +
> > > + vp = object->handle;
> > > + vhold(vp);
> > > + VM_OBJECT_UNLOCK(object);
> > > + vfslocked = VFS_LOCK_GIANT(vp->v_mount);
> > > + mp = NULL;
> > > + vn_start_write(vp, , V_WAIT);
> > > + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> > > +
> > > + /*
> > > + * Decrement the object's writemappings, by swapping the start
> > > + * and end arguments for vnode_pager_update_writecount(). If
> > > + * there was not a race with vnode reclaimation, then the
> > > + * vnode's v_writecount is decremented.
> > > + */
> > > + vnode_pager_update_writecount(object, end, start);
> >
> > ... here, first `end' is passed, then `start'. Is this intentional?
> Did you read the comment right before the call ?
I wish to assert again that all changes based on static
analysis tools require a formal code review by at minimum:
a) An expert in static analysis tools or a group of people
selected who we consider language experts.
b) An area expert from the area that is being affected
We as a project look bad by not having this minimal
set of code reviews in place for things that are coming
from a source that is frought with introducing bad
change.
Regards,
Rod
> > PVS Studio complains:
> >
> > /usr/src/sys/vm/vnode_pager.c:1584:1: warning: V764 Possible
> > incorrect order of arguments passed to 'vnode_pager_update_writecount'
> > function: 'end' and 'start'.
> >
> > > + VOP_UNLOCK(vp, 0);
> > > + vdrop(vp);
> > > + if (mp != NULL)
> > > + vn_finished_write(mp);
> > > + VFS_UNLOCK_GIANT(vfslocked);
> > > +}
>
>
--
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
On Mon, May 27, 2019 at 09:52:56AM +, Alexey Dokuchaev wrote:
> On Thu, Feb 23, 2012 at 09:07:16PM +, Konstantin Belousov wrote:
> > New Revision: 232071
> > URL: http://svn.freebsd.org/changeset/base/232071
> >
> > Log:
> > Account the writeable shared mappings backed by file in the vnode
> > v_writecount. Keep the amount of the virtual address space used by
> > the mappings in the new vm_object un_pager.vnp.writemappings
> > counter. The vnode v_writecount is incremented when writemappings gets
> > non-zero value, and decremented when writemappings is returned to
> > zero.
> >
> > Writeable shared vnode-backed mappings are accounted for in vm_mmap(),
> > and vm_map_insert() is instructed to set MAP_ENTRY_VN_WRITECNT flag on
> > the created map entry. During deferred map entry deallocation,
> > vm_map_process_deferred() checks for MAP_ENTRY_VN_WRITECOUNT and
> > decrements writemappings for the vm object.
> >
> > Now, the writeable mount cannot be demoted to read-only while
> > writeable shared mappings of the vnodes from the mount point
> > exist. Also, execve(2) fails for such files with ETXTBUSY, as it
> > should be.
> >
> > ...
> > Modified: head/sys/vm/vnode_pager.c
> > ==
> > --- head/sys/vm/vnode_pager.c Thu Feb 23 20:58:52 2012
> > (r232070)
> > +++ head/sys/vm/vnode_pager.c Thu Feb 23 21:07:16 2012
> > (r232071)
> > @@ -1215,3 +1222,81 @@ vnode_pager_undirty_pages(vm_page_t *ma,
> > }
> > VM_OBJECT_UNLOCK(obj);
> > }
> > +
> > +void
> > +vnode_pager_update_writecount(vm_object_t object, vm_offset_t start,
> > +vm_offset_t end)
>
> So, it is first `start, then `end', but below...
>
> > +{
> > + struct vnode *vp;
> > + vm_ooffset_t old_wm;
> > +
> > + ...
> > +}
> > +
> > +void
> > +vnode_pager_release_writecount(vm_object_t object, vm_offset_t start,
> > +vm_offset_t end)
> > +{
> > + struct vnode *vp;
> > + struct mount *mp;
> > + vm_offset_t inc;
> > + int vfslocked;
> > +
> > + VM_OBJECT_LOCK(object);
> > +
> > + /*
> > +* First, recheck the object type to account for the race when
> > +* the vnode is reclaimed.
> > +*/
> > + if (object->type != OBJT_VNODE) {
> > + VM_OBJECT_UNLOCK(object);
> > + return;
> > + }
> > +
> > + /*
> > +* Optimize for the case when writemappings is not going to
> > +* zero.
> > +*/
> > + inc = end - start;
> > + if (object->un_pager.vnp.writemappings != inc) {
> > + object->un_pager.vnp.writemappings -= inc;
> > + VM_OBJECT_UNLOCK(object);
> > + return;
> > + }
> > +
> > + vp = object->handle;
> > + vhold(vp);
> > + VM_OBJECT_UNLOCK(object);
> > + vfslocked = VFS_LOCK_GIANT(vp->v_mount);
> > + mp = NULL;
> > + vn_start_write(vp, , V_WAIT);
> > + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> > +
> > + /*
> > +* Decrement the object's writemappings, by swapping the start
> > +* and end arguments for vnode_pager_update_writecount(). If
> > +* there was not a race with vnode reclaimation, then the
> > +* vnode's v_writecount is decremented.
> > +*/
> > + vnode_pager_update_writecount(object, end, start);
>
> ... here, first `end' is passed, then `start'. Is this intentional?
Did you read the comment right before the call ?
>
> PVS Studio complains:
>
> /usr/src/sys/vm/vnode_pager.c:1584:1: warning: V764 Possible
> incorrect order of arguments passed to 'vnode_pager_update_writecount'
> function: 'end' and 'start'.
>
> > + VOP_UNLOCK(vp, 0);
> > + vdrop(vp);
> > + if (mp != NULL)
> > + vn_finished_write(mp);
> > + VFS_UNLOCK_GIANT(vfslocked);
> > +}
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r232071 - head/sys/vm
On Thu, Feb 23, 2012 at 09:07:16PM +, Konstantin Belousov wrote:
> New Revision: 232071
> URL: http://svn.freebsd.org/changeset/base/232071
>
> Log:
> Account the writeable shared mappings backed by file in the vnode
> v_writecount. Keep the amount of the virtual address space used by
> the mappings in the new vm_object un_pager.vnp.writemappings
> counter. The vnode v_writecount is incremented when writemappings gets
> non-zero value, and decremented when writemappings is returned to
> zero.
>
> Writeable shared vnode-backed mappings are accounted for in vm_mmap(),
> and vm_map_insert() is instructed to set MAP_ENTRY_VN_WRITECNT flag on
> the created map entry. During deferred map entry deallocation,
> vm_map_process_deferred() checks for MAP_ENTRY_VN_WRITECOUNT and
> decrements writemappings for the vm object.
>
> Now, the writeable mount cannot be demoted to read-only while
> writeable shared mappings of the vnodes from the mount point
> exist. Also, execve(2) fails for such files with ETXTBUSY, as it
> should be.
>
> ...
> Modified: head/sys/vm/vnode_pager.c
> ==
> --- head/sys/vm/vnode_pager.c Thu Feb 23 20:58:52 2012(r232070)
> +++ head/sys/vm/vnode_pager.c Thu Feb 23 21:07:16 2012(r232071)
> @@ -1215,3 +1222,81 @@ vnode_pager_undirty_pages(vm_page_t *ma,
> }
> VM_OBJECT_UNLOCK(obj);
> }
> +
> +void
> +vnode_pager_update_writecount(vm_object_t object, vm_offset_t start,
> +vm_offset_t end)
So, it is first `start, then `end', but below...
> +{
> + struct vnode *vp;
> + vm_ooffset_t old_wm;
> +
> + ...
> +}
> +
> +void
> +vnode_pager_release_writecount(vm_object_t object, vm_offset_t start,
> +vm_offset_t end)
> +{
> + struct vnode *vp;
> + struct mount *mp;
> + vm_offset_t inc;
> + int vfslocked;
> +
> + VM_OBJECT_LOCK(object);
> +
> + /*
> + * First, recheck the object type to account for the race when
> + * the vnode is reclaimed.
> + */
> + if (object->type != OBJT_VNODE) {
> + VM_OBJECT_UNLOCK(object);
> + return;
> + }
> +
> + /*
> + * Optimize for the case when writemappings is not going to
> + * zero.
> + */
> + inc = end - start;
> + if (object->un_pager.vnp.writemappings != inc) {
> + object->un_pager.vnp.writemappings -= inc;
> + VM_OBJECT_UNLOCK(object);
> + return;
> + }
> +
> + vp = object->handle;
> + vhold(vp);
> + VM_OBJECT_UNLOCK(object);
> + vfslocked = VFS_LOCK_GIANT(vp->v_mount);
> + mp = NULL;
> + vn_start_write(vp, , V_WAIT);
> + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> +
> + /*
> + * Decrement the object's writemappings, by swapping the start
> + * and end arguments for vnode_pager_update_writecount(). If
> + * there was not a race with vnode reclaimation, then the
> + * vnode's v_writecount is decremented.
> + */
> + vnode_pager_update_writecount(object, end, start);
... here, first `end' is passed, then `start'. Is this intentional?
PVS Studio complains:
/usr/src/sys/vm/vnode_pager.c:1584:1: warning: V764 Possible
incorrect order of arguments passed to 'vnode_pager_update_writecount'
function: 'end' and 'start'.
> + VOP_UNLOCK(vp, 0);
> + vdrop(vp);
> + if (mp != NULL)
> + vn_finished_write(mp);
> + VFS_UNLOCK_GIANT(vfslocked);
> +}
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
