* [bug report] ext4: data=journal: fixes for ext4_page_mkwrite()
@ 2020-10-30 11:47 Dan Carpenter
2020-10-30 17:29 ` Mauricio Faria de Oliveira
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-10-30 11:47 UTC (permalink / raw)
To: mfo; +Cc: linux-ext4
Hello Mauricio Faria de Oliveira,
The patch 64a9f1449950: "ext4: data=journal: fixes for
ext4_page_mkwrite()" from Oct 5, 2020, leads to the following static
checker warning:
fs/ext4/inode.c:6136 ext4_page_mkwrite()
error: uninitialized symbol 'get_block'.
fs/ext4/inode.c
6040 vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
6041 {
6042 struct vm_area_struct *vma = vmf->vma;
6043 struct page *page = vmf->page;
6044 loff_t size;
6045 unsigned long len;
6046 int err;
6047 vm_fault_t ret;
6048 struct file *file = vma->vm_file;
6049 struct inode *inode = file_inode(file);
6050 struct address_space *mapping = inode->i_mapping;
6051 handle_t *handle;
6052 get_block_t *get_block;
^^^^^^^^^^^^^^^^^^^^^^
6053 int retries = 0;
6054
6055 if (unlikely(IS_IMMUTABLE(inode)))
6056 return VM_FAULT_SIGBUS;
6057
6058 sb_start_pagefault(inode->i_sb);
6059 file_update_time(vma->vm_file);
6060
6061 down_read(&EXT4_I(inode)->i_mmap_sem);
6062
6063 err = ext4_convert_inline_data(inode);
6064 if (err)
6065 goto out_ret;
6066
6067 /*
6068 * On data journalling we skip straight to the transaction handle:
6069 * there's no delalloc; page truncated will be checked later; the
6070 * early return w/ all buffers mapped (calculates size/len) can't
6071 * be used; and there's no dioread_nolock, so only ext4_get_block.
6072 */
6073 if (ext4_should_journal_data(inode))
6074 goto retry_alloc;
^^^^^^^^^^^^^^^^
This goto is new.
6075
6076 /* Delalloc case is easy... */
6077 if (test_opt(inode->i_sb, DELALLOC) &&
6078 !ext4_nonda_switch(inode->i_sb)) {
6079 do {
6080 err = block_page_mkwrite(vma, vmf,
6081 ext4_da_get_block_prep);
6082 } while (err == -ENOSPC &&
6083 ext4_should_retry_alloc(inode->i_sb, &retries));
6084 goto out_ret;
6085 }
6086
6087 lock_page(page);
6088 size = i_size_read(inode);
6089 /* Page got truncated from under us? */
6090 if (page->mapping != mapping || page_offset(page) > size) {
6091 unlock_page(page);
6092 ret = VM_FAULT_NOPAGE;
6093 goto out;
6094 }
6095
6096 if (page->index == size >> PAGE_SHIFT)
6097 len = size & ~PAGE_MASK;
6098 else
6099 len = PAGE_SIZE;
6100 /*
6101 * Return if we have all the buffers mapped. This avoids the need to do
6102 * journal_start/journal_stop which can block and take a long time
6103 *
6104 * This cannot be done for data journalling, as we have to add the
6105 * inode to the transaction's list to writeprotect pages on commit.
6106 */
6107 if (page_has_buffers(page)) {
6108 if (!ext4_walk_page_buffers(NULL, page_buffers(page),
6109 0, len, NULL,
6110 ext4_bh_unmapped)) {
6111 /* Wait so that we don't change page under IO */
6112 wait_for_stable_page(page);
6113 ret = VM_FAULT_LOCKED;
6114 goto out;
6115 }
6116 }
6117 unlock_page(page);
6118 /* OK, we need to fill the hole... */
6119 if (ext4_should_dioread_nolock(inode))
6120 get_block = ext4_get_block_unwritten;
6121 else
6122 get_block = ext4_get_block;
^^^^^^^^^^^^^^^^^^^^^^^^^^
Initialized here.
6123 retry_alloc:
6124 handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
6125 ext4_writepage_trans_blocks(inode));
6126 if (IS_ERR(handle)) {
6127 ret = VM_FAULT_SIGBUS;
6128 goto out;
6129 }
6130 /*
6131 * Data journalling can't use block_page_mkwrite() because it
6132 * will set_buffer_dirty() before do_journal_get_write_access()
6133 * thus might hit warning messages for dirty metadata buffers.
6134 */
6135 if (!ext4_should_journal_data(inode)) {
6136 err = block_page_mkwrite(vma, vmf, get_block);
^^^^^^^^^
Smatch warning here.
6137 } else {
6138 lock_page(page);
6139 size = i_size_read(inode);
6140 /* Page got truncated from under us? */
6141 if (page->mapping != mapping || page_offset(page) > size) {
6142 ret = VM_FAULT_NOPAGE;
6143 goto out_error;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] ext4: data=journal: fixes for ext4_page_mkwrite()
2020-10-30 11:47 [bug report] ext4: data=journal: fixes for ext4_page_mkwrite() Dan Carpenter
@ 2020-10-30 17:29 ` Mauricio Faria de Oliveira
0 siblings, 0 replies; 2+ messages in thread
From: Mauricio Faria de Oliveira @ 2020-10-30 17:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-ext4
Hi Dan,
Thanks for the static checker/analysis report.
On Fri, Oct 30, 2020 at 8:47 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Mauricio Faria de Oliveira,
>
> The patch 64a9f1449950: "ext4: data=journal: fixes for
> ext4_page_mkwrite()" from Oct 5, 2020, leads to the following static
> checker warning:
>
> fs/ext4/inode.c:6136 ext4_page_mkwrite()
> error: uninitialized symbol 'get_block'.
>
The conditionals on ext4_should_journal_data() intentionally prevent
the uninitialized case.
Apparently that has not been considered by the checker. This is the
associated code path:
...
// skip initialization
if (ext4_should_journal_data(inode))
goto retry_alloc;
...
// initialization
get_block = ...
...
retry_alloc:
...
// usage only if ! skip initialization
if (!ext4_should_journal_data(inode)) {
err = block_page_mkwrite(vma, vmf, get_block);
...
If a patch to initialize it to NULL is welcome, I'd be happy to send it out.
It should fail similarly (i.e., invalid instruction memory access for
the get_block function pointer)
_if_ a future patch misses and compromises the conditionals, but
should silence the checker.
cheers,
Mauricio
> fs/ext4/inode.c
> 6040 vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
> 6041 {
> 6042 struct vm_area_struct *vma = vmf->vma;
> 6043 struct page *page = vmf->page;
> 6044 loff_t size;
> 6045 unsigned long len;
> 6046 int err;
> 6047 vm_fault_t ret;
> 6048 struct file *file = vma->vm_file;
> 6049 struct inode *inode = file_inode(file);
> 6050 struct address_space *mapping = inode->i_mapping;
> 6051 handle_t *handle;
> 6052 get_block_t *get_block;
> ^^^^^^^^^^^^^^^^^^^^^^
>
> 6053 int retries = 0;
> 6054
> 6055 if (unlikely(IS_IMMUTABLE(inode)))
> 6056 return VM_FAULT_SIGBUS;
> 6057
> 6058 sb_start_pagefault(inode->i_sb);
> 6059 file_update_time(vma->vm_file);
> 6060
> 6061 down_read(&EXT4_I(inode)->i_mmap_sem);
> 6062
> 6063 err = ext4_convert_inline_data(inode);
> 6064 if (err)
> 6065 goto out_ret;
> 6066
> 6067 /*
> 6068 * On data journalling we skip straight to the transaction handle:
> 6069 * there's no delalloc; page truncated will be checked later; the
> 6070 * early return w/ all buffers mapped (calculates size/len) can't
> 6071 * be used; and there's no dioread_nolock, so only ext4_get_block.
> 6072 */
> 6073 if (ext4_should_journal_data(inode))
> 6074 goto retry_alloc;
> ^^^^^^^^^^^^^^^^
> This goto is new.
>
> 6075
> 6076 /* Delalloc case is easy... */
> 6077 if (test_opt(inode->i_sb, DELALLOC) &&
> 6078 !ext4_nonda_switch(inode->i_sb)) {
> 6079 do {
> 6080 err = block_page_mkwrite(vma, vmf,
> 6081 ext4_da_get_block_prep);
> 6082 } while (err == -ENOSPC &&
> 6083 ext4_should_retry_alloc(inode->i_sb, &retries));
> 6084 goto out_ret;
> 6085 }
> 6086
> 6087 lock_page(page);
> 6088 size = i_size_read(inode);
> 6089 /* Page got truncated from under us? */
> 6090 if (page->mapping != mapping || page_offset(page) > size) {
> 6091 unlock_page(page);
> 6092 ret = VM_FAULT_NOPAGE;
> 6093 goto out;
> 6094 }
> 6095
> 6096 if (page->index == size >> PAGE_SHIFT)
> 6097 len = size & ~PAGE_MASK;
> 6098 else
> 6099 len = PAGE_SIZE;
> 6100 /*
> 6101 * Return if we have all the buffers mapped. This avoids the need to do
> 6102 * journal_start/journal_stop which can block and take a long time
> 6103 *
> 6104 * This cannot be done for data journalling, as we have to add the
> 6105 * inode to the transaction's list to writeprotect pages on commit.
> 6106 */
> 6107 if (page_has_buffers(page)) {
> 6108 if (!ext4_walk_page_buffers(NULL, page_buffers(page),
> 6109 0, len, NULL,
> 6110 ext4_bh_unmapped)) {
> 6111 /* Wait so that we don't change page under IO */
> 6112 wait_for_stable_page(page);
> 6113 ret = VM_FAULT_LOCKED;
> 6114 goto out;
> 6115 }
> 6116 }
> 6117 unlock_page(page);
> 6118 /* OK, we need to fill the hole... */
> 6119 if (ext4_should_dioread_nolock(inode))
> 6120 get_block = ext4_get_block_unwritten;
> 6121 else
> 6122 get_block = ext4_get_block;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> Initialized here.
>
> 6123 retry_alloc:
> 6124 handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
> 6125 ext4_writepage_trans_blocks(inode));
> 6126 if (IS_ERR(handle)) {
> 6127 ret = VM_FAULT_SIGBUS;
> 6128 goto out;
> 6129 }
> 6130 /*
> 6131 * Data journalling can't use block_page_mkwrite() because it
> 6132 * will set_buffer_dirty() before do_journal_get_write_access()
> 6133 * thus might hit warning messages for dirty metadata buffers.
> 6134 */
> 6135 if (!ext4_should_journal_data(inode)) {
> 6136 err = block_page_mkwrite(vma, vmf, get_block);
> ^^^^^^^^^
> Smatch warning here.
>
> 6137 } else {
> 6138 lock_page(page);
> 6139 size = i_size_read(inode);
> 6140 /* Page got truncated from under us? */
> 6141 if (page->mapping != mapping || page_offset(page) > size) {
> 6142 ret = VM_FAULT_NOPAGE;
> 6143 goto out_error;
>
> regards,
> dan carpenter
--
Mauricio Faria de Oliveira
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-10-30 17:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-30 11:47 [bug report] ext4: data=journal: fixes for ext4_page_mkwrite() Dan Carpenter
2020-10-30 17:29 ` Mauricio Faria de Oliveira
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.