Re: veriexec(4) maintenance

["J. Hannken-Illjes" <hannken%mailbox.org@localhost>]

On Tue, Dec 19, 2023 at 07:50:25AM +1030, Brett Lymn wrote:
> On Sat, Sep 02, 2023 at 08:57:03AM +0930, Brett Lymn wrote:
> > On Thu, Aug 31, 2023 at 10:24:07AM +0200, J. Hannken-Illjes wrote:
> > > 
> > > I'm short on time, some remarks:
> > > 
> > 
> > Thanks, I will have a closer look at these issues.  This could be where the deadlock I was
> > seeing is coming from.
> > 
> 
> OK, I have, finally, updated the diff in line with the comments (thanks
> again for the good feedback).

Brett,

first it is difficult to read a diff that also introduces a "notyet" operation
on file pages.

Looks like you replace global and item rw-locks with refcounts/signals.
I tried to understand the rationale behind and failed.  Your implementation
seems at least vulnerable to livelocks as the writer side is missing priority.

In general, what problem(s) are you trying to solve here?

What is wrong with a quite simple hashtable to hold fileid<->fingerprint mapping accessed as:

	mutex_enter table lock
	lookup fileid -> item
	mutex_enter item
	mutex_exit table lock

	while item state is busy
		cv_wait item cv

	if item state is unknown
		set item state busy
		mutex_exit item
		compute fingerprint
		mutex_enter item
		set item state valid/invalid
		cv_broadcast item cv

	Here the item state is either valid or invalid

	mutex_exit item

Some more problems (line numbers with your diff applied):

   476	veriexec_fp_calc(struct lwp *l, struct vnode *vp, int lock_state,
   477	    struct veriexec_file_entry *vfe, u_char *fp)
   478	{

   524			error = vn_rdwr(UIO_READ, vp, buf, len, offset,
   525					UIO_SYSSPACE, lock_state,

lock_state is sometimes VERIEXEC_UNLOCKED, sometimes IO_NODELOCKED.

  1629	veriexec_dump(struct lwp *l, prop_array_t rarray)
  1630	{
  1631		struct mount *mp;
  1632		mount_iterator_t *mpiter;
  1633	
  1634		mountlist_iterator_init(&mpiter);
  1635	
  1636		while ((mp = mountlist_iterator_trynext(mpiter)) != NULL) {
  1637			/* If it fails, the file-system is [being] unmounted. */
  1638			if (vfs_busy(mp) != 0)
  1639				continue;

This is completely wrong.  Operation mountlist_iterator_trynext() already
returns the mount busy and skips mounts currently unmounting or remounting.
Operation mountlist_iterator_next() was right or does this function get
called from inside unmount?

  1653	veriexec_flush(struct lwp *l)
  1654	{
  1655		struct mount *mp;
  1656		mount_iterator_t *mpiter;
  1657		int error = 0;
  1658	
  1659		mountlist_iterator_init(&mpiter);
  1660	
  1661		while ((mp = mountlist_iterator_trynext(mpiter)) != NULL) {
  1662			int lerror;
  1663	
  1664			/* If it fails, the file-system is [being] unmounted. */
  1665			if (vfs_busy(mp) != 0)
  1666				continue;

This is completely wrong, see above.

-- 
J. Hannken-Illjes