Hi Kees, On 06/16/2015 06:32 PM, Kees Cook wrote: > On Tue, Jun 16, 2015 at 3:03 AM, Michael Kerrisk (man-pages) > <mtk.manpages@xxxxxxxxx> wrote: >> On 06/04/2015 09:36 PM, Kees Cook wrote: >>> On Sat, May 9, 2015 at 1:54 AM, Michael Kerrisk (man-pages) >>> <mtk.manpages@xxxxxxxxx> wrote: >>>> ===== 2) Behavior puzzle (a) ===== >>>> >>>> The last sentence quoted from the man page was based on your sentence >>>> >>>> Writes to numeric sysctl entries must always be at file position 0 >>>> and the value must be fully contained in the buffer sent in the write >>>> syscall. >>>> >>>> So, I had interpreted /proc/sys/kernel/sysctl_writes_strict==1 to >>>> mean that if one writes into a numeric /proc/sys file at an offset >>>> other than zero, the write() will fail with some kind of error. >>> >>> Reporting back an error wasn't something I'd tested before. Looking at >>> the code again now, it should be possible make this change. >>> Regardless, in the case of the numeric value error condition, it's the >>> same as the "past the end" string error condition: "Anything written >>> beyond the maximum length of the value buffer will be ignored." i.e. >>> anything other than file offset 0 is considered "past the end of the >>> buffer" for a numeric value and is ignored. >>> >>>> But this seems not to be the case. Instead, the write() succeeds, >>>> but the file is left unmodified. That's surprising, I find. So, I'm >>>> wondering whether the implementation deviates from your intention. >>>> >>>> There's a test program below, which takes arguments as follows >>>> >>>> ./a.out pathname offset string >>> >>> I have tests in tools/testing/selftests/sysctl for checking the >>> various behaviors too. They don't actually examine any error >>> conditions from the sysctl writing itself. It should be simple to make >>> sysctl_writes_strict failures return an error, though. >> >> So, what do you think: is it *desirable* to make sysctl_writes_strict >> failures return an error? > > I think it would be desirable, yes. I want to improve the tests to add > error checking first, so I can make sure the change doesn't introduce > anything unexpected. The fix is simple, but since the code is a little > twisty, I want to be careful. Okay. >>>> ===== 2) Behavior puzzle (b) ===== >>>> >>>> In commit f88083005ab319abba5d0b2e4e997558245493c8, there is this note: >>>> >>>> This adds the sysctl kernel.sysctl_writes_strict to control the write >>>> behavior. The default (0) reports when VFS position is non-0 on a >>>> write, but retains legacy behavior, -1 disables the warning, and 1 >>>> enables the position-respecting behavior. >>>> >>>> The long-term plan here is to wait for userspace to be fixed in response >>>> to the new warning and to then switch the default kernel behavior to the >>>> new position-respecting behavior. >>>> >>>> (That last para was added to the commit message by AKPM, I see.) >>>> >>>> But, I wonder here whether /proc/sys/kernel/sysctl_writes_strict==0 >>>> is going to help with the long-term plan. The problem is that in >>>> warn_sysctl_write(), pr_warn_once() is used. This means that only >>>> the first offending user-space application that writes to *any* >>>> /proc/sys file will generate the printk warning. If that application >>>> isn't fixed, then none of the other "broken" applications will be >>>> discovered. It therefore seems possible that it could be a very long >>>> time before we could "switch the default kernel behavior to the >>>> new position-respecting behavior". >>>> >>>> Looking over old mails >>>> (http://thread.gmane.org/gmane.linux.kernel/1695177/focus=23240), >>>> I see that you're aware of the problem, but it seems to me that >>>> the switch to pr_warn_once() (for fear of spamming the log) likely >>>> dooms the long-term plan to failure. Your thoughts? >>> >>> In actual regular use, the situation that triggers the warning should >>> be vanishingly rare, but the condition can be trivially met by someone >>> intending to hit it for the purposes of filling log files. As such, it >>> makes sense to me to use _once to avoid spamming, but still catch a >>> rare usage under normal conditions. >> >> So, I'm not clear whether you think I'm wrong or not ;-). >> Do you disagree with my point that this approach may doom >> the long-term project to failure? (That was my main point.) > > Sorry! No, I don't think using pr_warn_once() will doom the > transition. I think that if we see the warning, we need to investigate > what's using sysctl that way. If we never see it, then we can switch > the default. (Using _once protects against log spamming.) I would > basically expect to never see this warning, but akpm wanted to be very > cautious, which I can't argue with. :) Okay. The man-pages text above will go out with the next release. Thanks for your help! Cheers, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
