Bug 194985 - getdtablecount new syscall from openbsd
Summary: getdtablecount new syscall from openbsd
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-11-13 08:20 UTC by David CARLIER
Modified: 2019-01-21 09:58 UTC (History)
4 users (show)

See Also:


Attachments
Proposed patch (8.96 KB, text/plain)
2014-11-13 08:20 UTC, David CARLIER
no flags Details
Updated kern code patch (1.16 KB, patch)
2014-11-13 13:17 UTC, David CARLIER
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David CARLIER 2014-11-13 08:20:41 UTC
Created attachment 149353 [details]
Proposed patch

Returns the number of file descriptors per process.
Comment 1 Mateusz Guzik freebsd_committer 2014-11-13 10:07:23 UTC
What rationale is behind this?

If such syscall is really needed, it may be it will be fast enough to count bits set in the map.

Also what's up with this:
+	p = td->td_proc;
+	PROC_LOCK(p);
+	td->td_retval[0] = p->p_fd->fd_openfd;
+	PROC_UNLOCK(p);

proc lock does not protect file table nor fd_openfd.
Comment 2 David CARLIER 2014-11-13 10:22:31 UTC
https://github.com/HardenedBSD/hardenedBSD/issues/63

So mm@ would not need to silent getdtablecount calls anymore.
So right, FILEDESC_SLOCK should protect f_dp field enough I think ?
Comment 3 David CARLIER 2014-11-13 13:17:11 UTC
Created attachment 149376 [details]
Updated kern code patch
Comment 4 Mateusz Guzik freebsd_committer 2014-11-13 14:14:29 UTC
+	p = td->td_proc;
+	PROC_LOCK(p);
+	fdp = p->p_fd;
+	FILEDESC_SLOCK(fdp);
+	td->td_retval[0] = fdp->fd_openfd;
+	FILEDESC_SUNLOCK(fdp);
+	PROC_UNLOCK(p);

proc lock has no useful purpose here. p_fd can change in some cases during fork, but then the process is singlethreaded.

proc lock is a mutex with bound sleep, while filedesc lock has unbound sleep. As such, they cannot be taken in this order anyway.

As a side note, they happen to be taken in the opposide order, so this code gives 2 different opportunities for deadlocks.

The kernel would tell you that if you enabled WITNESS and INVARIANTS.

Lastly, I'm not a fan of counting fds if it can be avoided. As mentioned in my previous comment, the kernel maintains a bitmap of open descriptors. I would suspect counting set bits (= open descriptors) would be fast enough for real life cases.
Comment 5 David CARLIER 2014-11-13 14:38:53 UTC
Fair enough I ll prepare a new version then.
Comment 6 Mateusz Guzik freebsd_committer 2014-11-13 17:47:35 UTC
So to be clear, I'm not convinced a syscall like this is needed, but I'm not going to oppose inclusion in the tree, regardless of implementation. This also means I'm not going to commit it.
Comment 7 commit-hook freebsd_committer 2015-11-14 23:08:22 UTC
A commit references this bug:

Author: rodrigc
Date: Sat Nov 14 23:07:39 UTC 2015
New revision: 290835
URL: https://svnweb.freebsd.org/changeset/base/290835

Log:
  Implemtn getdtablecount() to count open file descriptors for current process.

  Use underlying sysctl implemented by mjg in r290473.

  PR:                    194985
  Reviewed by:           bapt, mjg
  Differential Revision: https://reviews.freebsd.org/D4084

Changes:
  head/lib/libopenbsd/Makefile
  head/lib/libopenbsd/getdtablecount.c
  head/lib/libopenbsd/unistd.h
Comment 8 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2019-01-21 09:58:08 UTC
There is a commit referencing this PR, but it's still not closed and has been inactive for some time. Closing the PR as fixed but feel free to re-open it if the issue hasn't been completely resolved.

Thanks