linux-ext4 - Re: ext4: fold ext4_generic_write_end() into ext4_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 2 Apr 2013 19:09:18 +0800
From:	Zheng Liu <gnehzuil.liu@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	wenqing.lz@...bao.com, linux-ext4@...r.kernel.org, kbuild@...org,
	Theodore Ts'o <tytso@....edu>
Subject: Re: ext4: fold ext4_generic_write_end() into ext4_write_end()

[Add Ted into cc list]

Hi Dan,

Thanks for pointing it out.

On Tue, Apr 02, 2013 at 11:11:44AM +0300, Dan Carpenter wrote:
> Hello Zheng Liu,
> 
> The patch 31e4045fda81: "ext4: fold ext4_generic_write_end() into 
> ext4_write_end()" from Mar 26, 2013, leads to the following warning:
> "fs/ext4/inode.c:1169 ext4_write_end()
> 	 warn: unsigned 'copied' is never less than zero."
> 
> 
>   1110  static int ext4_write_end(struct file *file,
>   1111                            struct address_space *mapping,
>   1112                            loff_t pos, unsigned len, unsigned copied,
> 
> copied is unsigned.
> 
>   1113                            struct page *page, void *fsdata)
>   1114  {
>   1115          handle_t *handle = ext4_journal_current_handle();
>   1116          struct inode *inode = mapping->host;
>   1117          int ret = 0, ret2;
>   1118          int i_size_changed = 0;
>   1119  
>   1120          trace_ext4_write_end(inode, pos, len, copied);
>   1121          if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
>   1122                  ret = ext4_jbd2_file_inode(handle, inode);
>   1123                  if (ret) {
>   1124                          unlock_page(page);
>   1125                          page_cache_release(page);
>   1126                          goto errout;
>   1127                  }
>   1128          }
>   1129  
>   1130          if (ext4_has_inline_data(inode))
>   1131                  copied = ext4_write_inline_data_end(inode, pos, len,
>   1132                                                      copied, page);
>   1133          else
>   1134                  copied = block_write_end(file, mapping, pos,
>   1135                                           len, copied, page, fsdata);
> 
> I don't think these functions return negative error codes.

Yes, I check these two functions and they don't return a negative error
code.

> 
>   1136  
>   1137          /*
>   1138           * No need to use i_size_read() here, the i_size
>   1139           * cannot change under us because we hole i_mutex.
>   1140           *
>   1141           * But it's important to update i_size while still holding page lock:
>   1142           * page writeout could otherwise come in and zero beyond i_size.
>   1143           */
>   1144          if (pos + copied > inode->i_size) {
>   1145                  i_size_write(inode, pos + copied);
>   1146                  i_size_changed = 1;
>   1147          }
>   1148  
>   1149          if (pos + copied > EXT4_I(inode)->i_disksize) {
>   1150                  /* We need to mark inode dirty even if
>   1151                   * new_i_size is less that inode->i_size
>   1152                   * but greater than i_disksize. (hint delalloc)
>   1153                   */
>   1154                  ext4_update_i_disksize(inode, (pos + copied));
>   1155                  i_size_changed = 1;
>   1156          }
>   1157          unlock_page(page);
>   1158          page_cache_release(page);
>   1159  
>   1160          /*
>   1161           * Don't mark the inode dirty under page lock. First, it unnecessarily
>   1162           * makes the holding time of page lock longer. Second, it forces lock
>   1163           * ordering of page lock and transaction start for journaling
>   1164           * filesystems.
>   1165           */
>   1166          if (i_size_changed)
>   1167                  ext4_mark_inode_dirty(handle, inode);
>   1168  
>   1169          if (copied < 0)
>   1170                  ret = copied;
> 
> copied is never less than zero because it's unsigned.

Yes, it should be removed.

Ted, please remove this check.

Thanks,
                                                - Zheng
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html