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
Hi Dan,
Thanks for the static checker/analysis report.
On Fri, Oct 30, 2020 at 8:47 AM Dan Carpenter <[email protected]> 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