[09/14] fs: don't change the address limit for ->write_iter in __kernel_write
diff mbox series

Message ID 20200528054043.621510-10-hch@lst.de
State New
Headers show
Series
  • [01/14] cachefiles: switch to kernel_write
Related show

Commit Message

Christoph Hellwig May 28, 2020, 5:40 a.m. UTC
If we write to a file that implements ->write_iter there is no need
to change the address limit if we send a kvec down.  Implement that
case, and prefer it over using plain ->write with a changed address
limit if available.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/read_write.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Linus Torvalds May 28, 2020, 6:43 p.m. UTC | #1
On Wed, May 27, 2020 at 10:41 PM Christoph Hellwig <hch@lst.de> wrote:
>
> -ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t *pos)
> +ssize_t __kernel_write(struct file *file, const void *buf, size_t count,
> +               loff_t *pos)

Please don't do these kinds of pointless whitespace changes.

If you have an actual 80x25 vt100 sitting in a corner, it's not really
conducive to kernel development any more.

Yes, yes, we'd like to have shorter lines for new code, but no, don't
do silly line breaks that just makes old code look and grep worse.

             Linus
Al Viro May 28, 2020, 7 p.m. UTC | #2
On Thu, May 28, 2020 at 07:40:38AM +0200, Christoph Hellwig wrote:
> If we write to a file that implements ->write_iter there is no need
> to change the address limit if we send a kvec down.  Implement that
> case, and prefer it over using plain ->write with a changed address
> limit if available.

Umm...  It needs a comment along the lines of "weird shits like
/dev/sg that currently check for uaccess_kernel() will just
have to make sure they never switch to ->write_iter()"
Christoph Hellwig May 29, 2020, 5:57 a.m. UTC | #3
On Thu, May 28, 2020 at 08:00:52PM +0100, Al Viro wrote:
> On Thu, May 28, 2020 at 07:40:38AM +0200, Christoph Hellwig wrote:
> > If we write to a file that implements ->write_iter there is no need
> > to change the address limit if we send a kvec down.  Implement that
> > case, and prefer it over using plain ->write with a changed address
> > limit if available.
> 
> Umm...  It needs a comment along the lines of "weird shits like
> /dev/sg that currently check for uaccess_kernel() will just
> have to make sure they never switch to ->write_iter()"

sg and hid has the uaccess_kernel because it accesses userspace memory not
in the range passed to it.  Something using write_iter/read_iter should
never access any memory outside the iter passed to.  rdma has it because
it uses write as a bidirectional interface, which obviously can't work at
all with an iter.  So I'm not sure what we should comment on, but if
you have a desire and a proposal for a comment I'll happily add it.
Christoph Hellwig May 29, 2020, 12:32 p.m. UTC | #4
On Thu, May 28, 2020 at 11:43:13AM -0700, Linus Torvalds wrote:
> On Wed, May 27, 2020 at 10:41 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > -ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t *pos)
> > +ssize_t __kernel_write(struct file *file, const void *buf, size_t count,
> > +               loff_t *pos)
> 
> Please don't do these kinds of pointless whitespace changes.
> 
> If you have an actual 80x25 vt100 sitting in a corner, it's not really
> conducive to kernel development any more.

I have real 80x25 xterms, as that allows me to comfortably fit 4 of
them onto my latop screen.
Christoph Hellwig May 29, 2020, 1:37 p.m. UTC | #5
On Fri, May 29, 2020 at 07:57:36AM +0200, Christoph Hellwig wrote:
> On Thu, May 28, 2020 at 08:00:52PM +0100, Al Viro wrote:
> > On Thu, May 28, 2020 at 07:40:38AM +0200, Christoph Hellwig wrote:
> > > If we write to a file that implements ->write_iter there is no need
> > > to change the address limit if we send a kvec down.  Implement that
> > > case, and prefer it over using plain ->write with a changed address
> > > limit if available.
> > 
> > Umm...  It needs a comment along the lines of "weird shits like
> > /dev/sg that currently check for uaccess_kernel() will just
> > have to make sure they never switch to ->write_iter()"
> 
> sg and hid has the uaccess_kernel because it accesses userspace memory not
> in the range passed to it.  Something using write_iter/read_iter should
> never access any memory outside the iter passed to.  rdma has it because
> it uses write as a bidirectional interface, which obviously can't work at
> all with an iter.  So I'm not sure what we should comment on, but if
> you have a desire and a proposal for a comment I'll happily add it.

And looking over all three again they actually comment why they
check uaccess_kernel.  More importantly if someone switched them to
the ->write_iter carelessly that means the uaccess outside of the range
would actually aways fail now as we didn't allow access to userspace
memory, so this should show up when testing instantly.
Logan Gunthorpe May 31, 2020, 11:59 p.m. UTC | #6
On 2020-05-29 6:32 a.m., Christoph Hellwig wrote:
> On Thu, May 28, 2020 at 11:43:13AM -0700, Linus Torvalds wrote:
>> On Wed, May 27, 2020 at 10:41 PM Christoph Hellwig <hch@lst.de> wrote:
>>>
>>> -ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t *pos)
>>> +ssize_t __kernel_write(struct file *file, const void *buf, size_t count,
>>> +               loff_t *pos)
>>
>> Please don't do these kinds of pointless whitespace changes.
>>
>> If you have an actual 80x25 vt100 sitting in a corner, it's not really
>> conducive to kernel development any more.
> 
> I have real 80x25 xterms, as that allows me to comfortably fit 4 of
> them onto my latop screen.

I second this. Doing work on a compact laptop is a legitimate use case
and we can't all lug around big monitors with our laptops. I also find
more terminals on a screen to be more productive.

I'd also like to make the point that I never thought the width limit was
all that related to the hardware. It's been widely accepted for ages
that it's easier to read narrower blocks of text (try reading a book on
a landscape tablet: it's very difficult and causes eye strain). This is
why newspapers and magazines have always laid out their text in columns
and professional websites limit the width of their content. They have
the hardware to write much longer lines but chose not to for
readability. (Sadly, the *one* news source that I respect that doesn't
do this is LWN and I have to resort to reader view in Firefox to make it
readable.)

Furthermore, I find enforcing a line length limit on newer coders is one
of the easiest ways to improve the readability of their code. Without
it, I've seen developers generate lines of code that don't even fit in
the full width of a standard monitor. Putting in a little extra effort
to try to be clear in a shorter line (or adding more lines) usually pays
off in spades for readability. Or, it at least gets them to start
thinking about readability as an important concern. 90% of the time it
is better to refactor code that doesn't fit comfortably within the line
length limit than it is to violate it.

I personally set my terminal size to 80 chars because I believe it helps
the readability of the code I write. It has nothing to do with the width
of my monitor or the amount of characters I could theoretically fit
across my screen.

Logan

Patch
diff mbox series

diff --git a/fs/read_write.c b/fs/read_write.c
index 3bcb084f160de..8cfca5f8fc3ce 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -489,10 +489,9 @@  static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t
 }
 
 /* caller is responsible for file_start_write/file_end_write */
-ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t *pos)
+ssize_t __kernel_write(struct file *file, const void *buf, size_t count,
+		loff_t *pos)
 {
-	mm_segment_t old_fs;
-	const char __user *p;
 	ssize_t ret;
 
 	if (!(file->f_mode & FMODE_WRITE))
@@ -500,18 +499,29 @@  ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t
 	if (!(file->f_mode & FMODE_CAN_WRITE))
 		return -EINVAL;
 
-	old_fs = get_fs();
-	set_fs(KERNEL_DS);
-	p = (__force const char __user *)buf;
 	if (count > MAX_RW_COUNT)
 		count =  MAX_RW_COUNT;
-	if (file->f_op->write)
-		ret = file->f_op->write(file, p, count, pos);
-	else if (file->f_op->write_iter)
-		ret = new_sync_write(file, p, count, pos);
-	else
+	if (file->f_op->write_iter) {
+		struct kvec iov = { .iov_base = (void *)buf, .iov_len = count };
+		struct kiocb kiocb;
+		struct iov_iter iter;
+
+		init_sync_kiocb(&kiocb, file);
+		kiocb.ki_pos = *pos;
+		iov_iter_kvec(&iter, WRITE, &iov, 1, count);
+		ret = file->f_op->write_iter(&kiocb, &iter);
+		if (ret > 0)
+			*pos = kiocb.ki_pos;
+	} else if (file->f_op->write) {
+		mm_segment_t old_fs = get_fs();
+
+		set_fs(KERNEL_DS);
+		ret = file->f_op->write(file, (__force const char __user *)buf,
+				count, pos);
+		set_fs(old_fs);
+	} else {
 		ret = -EINVAL;
-	set_fs(old_fs);
+	}
 	if (ret > 0) {
 		fsnotify_modify(file);
 		add_wchar(current, ret);