Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201030114737.GC3251003@mwanda>
In-Reply-To: <20201030114737.GC3251003@mwanda>
From:   Mauricio Faria de Oliveira <mfo@canonical.com>
Date:   Fri, 30 Oct 2020 14:29:55 -0300
Message-ID: <CAO9xwp2Z8gj1Ayr3BmNEpgR6325Udc2HHPr5LJafPqYBcVLshA@mail.gmail.com>
Subject: Re: [bug report] ext4: data=journal: fixes for ext4_page_mkwrite()
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     linux-ext4@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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