| 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
| ||
|
Message-ID:
<SJ0PR11MB5896E5F325ABE2CD6AF3F8CBC3C42@SJ0PR11MB5896.namprd11.prod.outlook.com>
Date: Thu, 20 Feb 2025 03:08:17 +0000
From: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
To: Dan Carpenter <dan.carpenter@...aro.org>, "oe-kbuild@...ts.linux.dev"
<oe-kbuild@...ts.linux.dev>
CC: "lkp@...el.com" <lkp@...el.com>, "oe-kbuild-all@...ts.linux.dev"
<oe-kbuild-all@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "Martin K. Petersen"
<martin.petersen@...cle.com>, "Sesidhar Baddela (sebaddel)"
<sebaddel@...co.com>, "Gian Carlo Boffa (gcboffa)" <gcboffa@...co.com>,
"Arulprabhu Ponnusamy (arulponn)" <arulponn@...co.com>, "Arun Easi (aeasi)"
<aeasi@...co.com>
Subject: RE: drivers/scsi/fnic/fdls_disc.c:263
fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting
On Friday, February 14, 2025 6:42 AM, Dan Carpenter <dan.carpenter@...aro.org> wrote:
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 128c8f96eb8638c060cd3532dc394d046ce64fe1
> commit: a63e78eb2b0f654b138abfc323f6bd7573e26145 scsi: fnic: Add support for fabric based solicited requests and responses
> config: i386-randconfig-141-20250214 (https://download.01.org/0day-ci/archive/20250214/202502141403.1PcpwyJp-lkp@intel.com/config)
> compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@...el.com>
> | Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> | Closes: https://lore.kernel.org/r/202502141403.1PcpwyJp-lkp@intel.com/
>
> smatch warnings:
> drivers/scsi/fnic/fdls_disc.c:263 fdls_schedule_oxid_free_retry_work() warn: inconsistent indenting
> drivers/scsi/fnic/fdls_disc.c:989 fdls_send_fabric_logo() warn: inconsistent indenting
> drivers/scsi/fnic/fdls_disc.c:1953 fnic_fdls_validate_and_get_frame_type() warn: inconsistent indenting
>
> vim +/cur_jiffies +166 drivers/scsi/fnic/fdls_disc.c
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 244 void fdls_schedule_oxid_free_retry_work(struct work_struct *work)
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 245 {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 246 struct fnic_oxid_pool_s *oxid_pool = container_of(work,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 247 struct fnic_oxid_pool_s, schedule_oxid_free_retry.work);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 248 struct fnic_iport_s *iport = container_of(oxid_pool,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 249 struct fnic_iport_s, oxid_pool);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 250 struct fnic *fnic = iport->fnic;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 251 struct reclaim_entry_s *reclaim_entry;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 252 unsigned long delay_j = msecs_to_jiffies(OXID_RECLAIM_TOV(iport));
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 253 int idx;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 254
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 255 spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
>
> Passing fnic->lock_flags is wrong. spin_lock_irqsave() is not nestable
> in the sense that the "flags" variable can't hold two values at once:
>
> orig_irq = save_original irq states and disable()
> orig_irq = save_original irq states and disable()
> restore(orig_irq)
> restore(orig_irq)
>
> The second restore(orig_irq) is not going to restore the original states
> it's going to leave them as disabled.
>
> If fnic->lock_flags is holding anything useful when this is called, then it
> is a bug or if anything uses it before we exit that will break us. Just
> declare "unsigned long flags;" locally.
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 256
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 257 for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 258
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 259 FNIC_FCS_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 260 "Schedule oxid free. oxid idx: %d\n", idx);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 261
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 262 spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 @263 reclaim_entry = (struct reclaim_entry_s *)
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 264 kzalloc(sizeof(struct reclaim_entry_s), GFP_KERNEL);
Thanks for this change, Dan. It looks good.
>
> The indenting is off. The normal way to write this is:
>
> reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL);
>
> Remove the cast and change the sizeof().
Thanks for this change, Dan. It looks good.
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 265 spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 266
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 267 if (!reclaim_entry) {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 268 FNIC_FCS_DBG(KERN_WARNING, fnic->lport->host, fnic->fnic_num,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 269 "Failed to allocate memory for reclaim struct for oxid idx: 0x%x\n",
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 270 idx);
>
> Remove this DBG. The truth is that for as long as anyone can remember
> the kmalloc() allocator won't fail for tiny allocations. We're more
> likely to formalize this rule than we are to change it at this point.
> Still, we check for errors until that day arrives.
Thanks for this change, Dan. It looks good.
> But allocations are not supposed to print an error message on error.
> It's just messy and it doesn't add any information that isn't already
> in the stack traces that kmalloc() prints.
>
> I guess we can't call schedule_delayed_work() without holding the
> spin_lock? It's a strange thing.
>
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 271
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 272 schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry,
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 273 msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME));
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 274 spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 275 return;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 276 }
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 277
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 278 if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 279 reclaim_entry->oxid_idx = idx;
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 280 reclaim_entry->expires = round_jiffies(jiffies + delay_j);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 281 list_add_tail(&reclaim_entry->links, &oxid_pool->oxid_reclaim_list);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 282 schedule_delayed_work(&oxid_pool->oxid_reclaim_work, delay_j);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 283 } else {
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 284 /* unlikely scenario, free the allocated memory and continue */
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 285 kfree(reclaim_entry);
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 286 }
> a63e78eb2b0f65 Karan Tilak Kumar 2024-12-11 287 }
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
>
Would you like to send out a patch for these changes? If so, we can review and sign-off on it.
If you would like us to send out a patch, that's fine too; we will address these changes in a future patch.
Regards,
Karan (on behalf of the Cisco team)
Powered by blists - more mailing lists