Could you please incorporate the manual page change from D24814 into this one?

IMHO I think it's not worth the effort to make this knob-controllable and the old behavior should be gone: directories are not stored in an uniform format, it varies from file system to file system and I'd be really surprised if anything would depend on the old behavior: for starters, these application would have already been broken many years ago when we move to a different file system. Neither fsck_ffs nor fsck_msdosfs rely on being able to read() directories directly, and I don't see any legitimate reason that an application should do it either.

The EISDIR behavior is standard compliant and is already adopted by at least macOS, Linux and OpenBSD, by the way.

Further discussion leaves me content with this patch, without a a sysctl to revert to the old behaviour.

In D24596#546024, @delphij wrote:

Could you please incorporate the manual page change from D24814 into this one?

Definitely, will do in the next iteration.

In D24596#546208, @emaste wrote:

Further discussion leaves me content with this patch, without a a sysctl to revert to the old behaviour.

It's somewhat tempting to leave a kernel config option behind to re-enable it and only axe the functionality everywhere that isn't ufs. I imagine Kirk's probably sometimes examining specifics that aren't necessarily reachable with readdir; this option would require basically no maintenance, as the path from vn_io_fault to ufs doesn't require any change and Kirk's the maintainer of the other side.

read(2) on a directory seems only useful for educational or debugging purposes. These purposes are better served by a different API.

read(2) on a directory may expose information about files that have been deleted, which is a security problem.

Given that some applications (such as Git) may depend on or be statically configured to depend on read(2) on a directory returning an error, a global knob seems unwise. Removing the functionality entirely seems better than adding a complicated enabling mechanism that will not be used much. So I agree with the current patch (plus a man page change).

Go for it!

Probably grep(1) should be updated after this change. It has -d option to specify which action to to take on processing directories. From manpage:

-d ACTION, --directories=ACTION
       If an input file is a directory, use ACTION to process it.  By
       default, ACTION is read, which means that directories are read
       just as if they were ordinary files.  If ACTION is skip,
       directories are silently skipped.  If ACTION is recurse, grep
       reads all files under each directory, recursively; this is
       equivalent to the -r option.

In D24596#547104, @mateusz_serveraptor.com wrote:

Probably grep(1) should be updated after this change. It has -d option to specify which action to to take on processing directories. From manpage:

-d ACTION, --directories=ACTION
       If an input file is a directory, use ACTION to process it.  By
       default, ACTION is read, which means that directories are read
       just as if they were ordinary files.  If ACTION is skip,
       directories are silently skipped.  If ACTION is recurse, grep
       reads all files under each directory, recursively; this is
       equivalent to the -r option.

grep doesn't actually read directories for anything useful (and can't for ZFS), so this change is a nop as far as GREP is concerned.

I will likely commit this in three parts if we can all agree on this:

1.) ZFS rejection of read(2) of a dirfd
2.) sysctl without the priv check
3.) add the priv check

I think it would be a good idea to MFC just the sysctl (default flipped to ON to not violate POLA) so that there's the option on stable/12 to turn the behavior off in some form for those that don't want it.

Now that I have had time to go play with this on a "Linux" system, I am afraid I object to this change. Your doing it because you don't like the occasional splat of "foo is a binary file" when you run a grep *, you should see the results when EISDIR is returned. These types of greps on a linux system return an error message for every directory in the directory you ran the grep in, far more noise than your small issue, and for many of us this is gona be a PITA/POLA change. I hadnt thought much about it cause I alias 'grep' 'grep -d skip' and do not see your issue or my issue normally on linux.

I think to rectifier your issue you should probably do the same I have done...

In D24596#547790, @rgrimes wrote:

Now that I have had time to go play with this on a "Linux" system, I am afraid I object to this change. Your doing it because you don't like the occasional splat of "foo is a binary file" when you run a grep *, you should see the results when EISDIR is returned. These types of greps on a linux system return an error message for every directory in the directory you ran the grep in, far more noise than your small issue, and for many of us this is gona be a PITA/POLA change. I hadnt thought much about it cause I alias 'grep' 'grep -d skip' and do not see your issue or my issue normally on linux.

I think to rectifier your issue you should probably do the same I have done...

I'm afraid I have no idea what you're talking about here...grep isn't my problem, my problems are:

1. The security implications of being able to read(2) a dirfd,
2. Consistency with how most modern kernels behave,

2a) the amount of time I waste hunting down application bugs because of that

There was probably a 3, but it's late and a bit worn down from recent discussions.

In D24596#547205, @imp wrote:

In D24596#547104, @mateusz_serveraptor.com wrote:

Probably grep(1) should be updated after this change. It has -d option to specify which action to to take on processing directories. From manpage:

-d ACTION, --directories=ACTION
       If an input file is a directory, use ACTION to process it.  By
       default, ACTION is read, which means that directories are read
       just as if they were ordinary files.  If ACTION is skip,
       directories are silently skipped.  If ACTION is recurse, grep
       reads all files under each directory, recursively; this is
       equivalent to the -r option.

grep doesn't actually read directories for anything useful (and can't for ZFS), so this change is a nop as far as GREP is concerned.

Thats actually wrong, grep shall now spew forth tons of EISDIR errors when run after this change and a grep * is done.

In D24596#547795, @kevans wrote:

In D24596#547790, @rgrimes wrote:

Now that I have had time to go play with this on a "Linux" system, I am afraid I object to this change. Your doing it because you don't like the occasional splat of "foo is a binary file" when you run a grep *, you should see the results when EISDIR is returned. These types of greps on a linux system return an error message for every directory in the directory you ran the grep in, far more noise than your small issue, and for many of us this is gona be a PITA/POLA change. I hadnt thought much about it cause I alias 'grep' 'grep -d skip' and do not see your issue or my issue normally on linux.

I think to rectifier your issue you should probably do the same I have done...

I'm afraid I have no idea what you're talking about here...grep isn't my problem, my problems are:

1. The security implications of being able to read(2) a dirfd,
2. Consistency with how most modern kernels behave,

2a) the amount of time I waste hunting down application bugs because of that

There was probably a 3, but it's late and a bit worn down from recent discussions.

I can't find it in the bugzilla, so it must of been on the mail list, and I attributed it to you, which I guess it was not. But this change has a very visible change for users of grep *.

In D24596#547797, @rgrimes wrote:

I can't find it in the bugzilla, so it must of been on the mail list, and I attributed it to you, which I guess it was not. But this change has a very visible change for users of grep *.

I now vaguely recall responding to someone on the list about this- yes, I've found it annoying in the past on UFS when grep's called a directory a file, but this wasn't and hasn't been a consideration in this policy change.

These types of greps on a linux system return an error message for every directory in the directory you ran the grep in

So if we modify grep to not try to read directories and not spew errors, would that be a solution ?

Or is there some POSIX or similar rule that requires those errors 8-} ?

As for the impact on grep itself- yes, it does, but it's not clear to me that it's a negative impact.

• When you're grepping *, it seems unlikely that you're trying to find names that exist within directories in cwd.
• If you are trying to find names that exist, I would argue that grep * is the wrong tool for this anyways, as it won't work on many filesystems

In either case, you're getting mixed results right now. You're either wanting matches within files and spending time grepping directory entries, or you're wanting directory entries and wasting time matching files.

In D24596#547800, @pi wrote:

These types of greps on a linux system return an error message for every directory in the directory you ran the grep in

So if we modify grep to not try to read directories and not spew errors, would that be a solution ?

It wouldn't even take much modification, just changing the default to be -d skip.

Or is there some POSIX or similar rule that requires those errors 8-} ?

POSIX allows implementations to do what they want with read() on a directory and grep's behaviour follows from that, so I think it's within the spirit for us to show errors or not, as we see fit.

In D24596#548776, @emaste wrote:

In D24596#547800, @pi wrote:

These types of greps on a linux system return an error message for every directory in the directory you ran the grep in

So if we modify grep to not try to read directories and not spew errors, would that be a solution ?

It wouldn't even take much modification, just changing the default to be -d skip.

Or is there some POSIX or similar rule that requires those errors 8-} ?

POSIX allows implementations to do what they want with read() on a directory and grep's behaviour follows from that, so I think it's within the spirit for us to show errors or not, as we see fit.

Please do not change the default, that would just make our greps defaults different than linux and everyone else. Just "recommend" the use of grep -d skip in the UPDATE entry, the RELEASE NOTES, and perhaps by adding t as an example, aka commented out, in /root/csh.cshrc and /root/.profile, and perhaps in the usr/share/skel files.

In D24596#548787, @rgrimes wrote:

In D24596#548776, @emaste wrote:

In D24596#547800, @pi wrote:

These types of greps on a linux system return an error message for every directory in the directory you ran the grep in

So if we modify grep to not try to read directories and not spew errors, would that be a solution ?

It wouldn't even take much modification, just changing the default to be -d skip.

Or is there some POSIX or similar rule that requires those errors 8-} ?

POSIX allows implementations to do what they want with read() on a directory and grep's behaviour follows from that, so I think it's within the spirit for us to show errors or not, as we see fit.

Please do not change the default, that would just make our greps defaults different than linux and everyone else. Just "recommend" the use of grep -d skip in the UPDATE entry, the RELEASE NOTES, and perhaps by adding t as an example, aka commented out, in /root/csh.cshrc and /root/.profile, and perhaps in the usr/share/skel files.

That seems reasonable, though I do wonder if -d read as a default really makes sense anywhere else. The only linux filesystem I'm aware of that allows you to read() directories, I suspect a grep user wouldn't want to include that (directory statistics) by default and otherwise they're just getting lots of errors. OTOH, the error's a nice indicator that maybe you meant to recurse instead.

In D24596#548828, @delphij wrote:

In D24596#548787, @rgrimes wrote:

In D24596#548776, @emaste wrote:

POSIX allows implementations to do what they want with read() on a directory and grep's behaviour follows from that, so I think it's within the spirit for us to show errors or not, as we see fit.

Please do not change the default, that would just make our greps defaults different than linux and everyone else. Just "recommend" the use of grep -d skip in the UPDATE entry, the RELEASE NOTES, and perhaps by adding t as an example, aka commented out, in /root/csh.cshrc and /root/.profile, and perhaps in the usr/share/skel files.

Can we please, please, please refrain from turning /usr/share/skel files into an encyclopedia describing every corner cases of command line utilities when they are already properly documented? The user do not need this information, even if they do, they are not going to read the skeleton but to read manual page.

I can remove this pre-commit if consensus moves in that direction.

By the way, bsdgrep doesn't really give any error when grep'ing directory. tmpfs already returns EISDIR when reading, and here is my test:

$ /usr/bin/grep . /tmp
$ /usr/local/bin/grep . /tmp
grep: /tmp: Is a directory

(the latter is GNU grep).

Something odd must be going on here, because it definitely does warn:

root@viper:/usr/src/bin/sh# grep -V
grep (BSD grep) 2.6.0-FreeBSD
root@viper:/usr/src/bin/sh# grep 'test' .
grep: .: Is a directory

In D24596#548835, @kevans wrote:

In D24596#548828, @delphij wrote:

By the way, bsdgrep doesn't really give any error when grep'ing directory. tmpfs already returns EISDIR when reading, and here is my test:

$ /usr/bin/grep . /tmp
$ /usr/local/bin/grep . /tmp
grep: /tmp: Is a directory

(the latter is GNU grep).

Something odd must be going on here, because it definitely does warn:

root@viper:/usr/src/bin/sh# grep -V
grep (BSD grep) 2.6.0-FreeBSD
root@viper:/usr/src/bin/sh# grep 'test' .
grep: .: Is a directory

Ambient GREP_OPTIONS environment variable?

I can remove this pre-commit if consensus moves in that direction.

Even if it's getting included I would commit it separately.

Are there any other outstanding concerns that haven't been addressed? I think consensus is that the skel/ changes will simply never get committed.