Two changes will be made before landing this:

This looks pretty good.

Fixed accidentally adding an unrelated commit

lib/msun: Added fmaximum and fminimum families. Tests and man page

Sorry for the delay, exam season is rough this year, but once I'm done I will follow up with the fminimum and man pages

Do you plan to follow up on this before the start of GSoC?

Looks pretty reasonable, though I'll let Steve Kargl have the last word.
Needs a man page.

This will end up being a port.

In D41679#949984, @manu wrote:

In D41679#949804, @imp wrote:

sys/contrib is good for this, but sys/gnu might be even better since it is GPL'd.

I don't think that we want new GPL'ed code in base ?

In D41679#949804, @imp wrote:

sys/contrib is good for this, but sys/gnu might be even better since it is GPL'd.

In D41679#949804, @imp wrote:

sys/contrib is good for this, but sys/gnu might be even better since it is GPL'd.

sys/contrib is good for this, but sys/gnu might be even better since it is GPL'd.

ld: error: undefined symbol: arp_ifinit

this issue is fixed in latest code in a commit last week https://github.com/freebsd/freebsd-src/commit/5684c8783b64e33f0dab058126b36776adcc8e82

In D20967#934539, @bz wrote:

Can you please point to correct kernel config to be used?

updated the variable declaration and scope considering NO-INET, NO-INET6, NO-IP.

In D20967#934400, @bz wrote:

Someone should do (at least) an amd64 universe build for this to make sure the NO-INET NO-INET6 NO-IP (do we still have that) builds are surviving.

- ident		GENERIC
+ include		LINT-NOINET
+ include		LINT-NOINET6
+ include		LINT-NOIP
+ ident		MYKERNEL

Someone should do (at least) an amd64 universe build for this to make sure the NO-INET NO-INET6 NO-IP (do we still have that) builds are surviving.

LGTM, but as I don't have a src bit I can't commit this. Anybody else willing to do that?

Melifaro's point about avoiding parsing in the kernel is a good one. What do you think could be improved here?

• Implemented recommended changes
• Made style changes
• modifed the rules_check function to traverse the list in reverse order and stop when first applicable rule (matching IP address) is found. Since, rules defined later determine have higer priority, checking last matching rule is enough.

If you're going to change format, why not use tree, as it feels natural for sysctl. One existing example would be dev.pcm.<number> so following that, we could have security.mac.ipacl.<jid>.<rule> and .family and .address (or maybe .range) as leafs. It is not set in stone that it has to be like that, but given it's sysctl, it feels more natural to me, so please give it a thought. Also, as it is about jail, it would also feel more natural to have this tunables under security.jail.<jid>

The @ character is visually quite large and makes it hard to scan and read for a human.

Manual page LGTM now, will rereview once the source code is finalized.

Overall is a really nice addition and I'd love to have it in base. I have one concern on the rule import/export implementation - happy to discuss this further & left a couple of non-critical comments on the code.
Thank you for working on this!

• rebased the code on top of latest src changes
• made changes to man page as suggested in comments

Hi,
thanks for the comments.
I'll test my patch and wil apply the suggested changes in man page.
Thanks

In D20967#923964, @pauamma_gundo.com wrote:

Some nits in the manual page, which can probably be fixed in a follow-up commit (with .Dd bumped) if the code still works.

Some nits in the manual page, which can probably be fixed in a follow-up commit (with .Dd bumped) if the code still works.

In D20967#632260, @dch wrote:

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

In D20967#632260, @dch wrote:

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

In D26243#587330, @asomers wrote:

Isn't audit_nfsarg_vnode1 the problem? You already know the path when you call AUDIT_ARG_UPATH1_VP, right?

In D26243#587174, @shivank wrote:

In D26243#587132, @mjg wrote:

Audit support for regular lookup starts with AUDIT_ARG_UPATH1_VP/AUDIT_ARG_UPATH2_VP without any vnodes locked. Later on visited vnodes get added with AUDIT_ARG_VNODE1/AUDIT_ARG_VNODE2 which only performs VOP_GETATTR (i.e. does *NOT* resolve any paths). Your code should follow the same scheme.

As you can see path resolving routines can take vnode locks on their own (modulo the smr case). This means they can't be called with locked vnodes to begin with, as otherwise you risk violating global lock ordering and consequently deadlocking the kernel.

The VOP_ISLOCKED routine is not entirely legal to call if you don't hold the lock. The name is perhaps misleading, but it can only reliably tell you that you have an exclusive lock or that *SOMEONE* has a shared lock (and it may be you). Or to put it differently, if you don't have the vnode locked but someone else has it shared locked, you will get non-0 and that's how you get the panic. Regardless of this problem, adding the call reduces performance and most notably suggests a bug on its own.

So the question is why are you calling here with any vnodes locked.

I wish to audit the canonical path of the file requested by the NFS clients. The requested path from the client is extracted in the NFS server using nfsrv_parsename, but the vnode is locked in some NFS services. I thought of unlocking/relocking of vnode for path audit but Rick advised not to. That's why I had to call this locked vnode.

Thanks for your question which made me rethink the problem from scratch and I got a new idea for auditing path.

Hi @rmacklem and @asomers, if I use nfsvno_namei to get the canonical path for the client, I will not the need the AUDIT_ARG_UPATH1_VP.which will save me from all the trouble of passing locked vnode to vn_fullpath_global. Please provide your opinion on the same.

In D26243#587132, @mjg wrote:

Audit support for regular lookup starts with AUDIT_ARG_UPATH1_VP/AUDIT_ARG_UPATH2_VP without any vnodes locked. Later on visited vnodes get added with AUDIT_ARG_VNODE1/AUDIT_ARG_VNODE2 which only performs VOP_GETATTR (i.e. does *NOT* resolve any paths). Your code should follow the same scheme.

As you can see path resolving routines can take vnode locks on their own (modulo the smr case). This means they can't be called with locked vnodes to begin with, as otherwise you risk violating global lock ordering and consequently deadlocking the kernel.

The VOP_ISLOCKED routine is not entirely legal to call if you don't hold the lock. The name is perhaps misleading, but it can only reliably tell you that you have an exclusive lock or that *SOMEONE* has a shared lock (and it may be you). Or to put it differently, if you don't have the vnode locked but someone else has it shared locked, you will get non-0 and that's how you get the panic. Regardless of this problem, adding the call reduces performance and most notably suggests a bug on its own.

So the question is why are you calling here with any vnodes locked.

Audit support for regular lookup starts with AUDIT_ARG_UPATH1_VP/AUDIT_ARG_UPATH2_VP without any vnodes locked. Later on visited vnodes get added with AUDIT_ARG_VNODE1/AUDIT_ARG_VNODE2 which only performs VOP_GETATTR (i.e. does *NOT* resolve any paths). Your code should follow the same scheme.

I feel vfs_cache.c changes for making vn_fullpath_global work for optionally locked vnode are causing the trouble. Though I'm not sure what's the problem. I request Mateusz Guzik, @mjg to have a look at my vfs_cache.c changes. I would be grateful for your time.

The new code looks better. But grrr, there are two big problems:

1. It doesn't compile due to some recent changes on head. I suggest the following:
   • Remove the <rpc/rpc.h>, <sys/mount.h>, and <fs/nfs/*> includes from audit.h. In addition to fixing the compile failure, it's generally not recommended to include headers from other headers. Sometimes it's necessary, but it also causes header pollution, and slow build times. Instead of including those files, just forward declare struct nfsrv_descript; and struct kaudit_record;.
   • Add `<netinet/in.h>, <rpc/rpc.h>, <fs/nfs/nfsdport.h>, <fs/nfs/nfsproto.h>, and <fs/nfs/nfs.h> to audit_bsm_db.c
   • Add <rpc/rpc.h>, <fs/nfs/nfsport.h>, <fs/nfs/nfsproto.h>, and <fs/nfs/nfs.h> to audit.c

• merge vn_fullpath_any and vn_vptocnp with their locked counterpart to work for optionally locked vnodes.

I created a new review - D26243. Sorry for the trouble.

It was earlier being reviewed on D25869. But due to change of base revision, It was showing changes which were not mine. So, I created a new review here.