linux-kernel - Re: [PATCH v4] scsi: libsas: Fix the failure of adding phy with zero-address to port

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 27 Nov 2023 19:28:43 +0000
From:   John Garry <john.g.garry@...cle.com>
To:     yangxingui <yangxingui@...wei.com>, yanaijie@...wei.com,
        jejb@...ux.ibm.com, martin.petersen@...cle.com,
        damien.lemoal@...nsource.wdc.com
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linuxarm@...wei.com, prime.zeng@...ilicon.com,
        kangfenglong@...wei.com, chenxiang66@...ilicon.com
Subject: Re: [PATCH v4] scsi: libsas: Fix the failure of adding phy with
 zero-address to port

On 24/11/2023 02:27, yangxingui wrote:
>> We already do this in sas_ex_join_wide_port(), right?
> No, If the addr of ex_phy matches dev->parent, sas_ex_join_wide_port() 
> will not be called, but sas_add_parent_port() will be called  as follows:
> static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
> {
>          struct expander_device *ex = &dev->ex_dev;
>          struct ex_phy *ex_phy = &ex->ex_phy[phy_id];
>          struct domain_device *child = NULL;
>          int res = 0;
> 
>      <...>
>          /* Parent and domain coherency */
>          if (!dev->parent && sas_phy_match_port_addr(dev->port, ex_phy)) {
>                  sas_add_parent_port(dev, phy_id);
>                  return 0;
>          }
>          if (dev->parent && sas_phy_match_dev_addr(dev->parent, ex_phy)) {
>                  sas_add_parent_port(dev, phy_id);
>                  if (ex_phy->routing_attr == TABLE_ROUTING)
>                          sas_configure_phy(dev, phy_id, 
> dev->port->sas_addr, 1);
>                  return 0;
>          }
>      <...>
> }
> 
>>
>> I am not saying that what we do now does not have a problem - I am 
>> just trying to understand what currently happens
> 
> ok, because ex_phy->port is not set when calling sas_add_parent_port(), 
> when deleting phy from the parent wide port, it is not removed from the 
> phy_list of the parent wide port as follows:
> static void sas_unregister_devs_sas_addr(struct domain_device *parent,
>                                           int phy_id, bool last)
> {
>      <...>
>      // Since ex_phy->port is not set, this branch will not be enter

But then how does this ever work? It is because we follow path 
sas_rediscover_dev() -> sas_discover_new() -> sas_ex_discover_devices() 
-> sas_ex_discover_dev() -> sas_add_parent_port(), and not 
sas_rediscover_dev() -> sas_discover_new() -> sas_ex_join_wide_port()? 
If so, is that because ephy->sas_attached_phy == 0 in sas_discover_new() 
-> sas_ex_join_wide_port() and it fails?

BTW, about something mentioned earlier - adding the phy19 with SAS_ADDR 
= 0 ever to a sas_port seems wrong.

>          if (phy->port) {
>                  sas_port_delete_phy(phy->port, phy->phy);
>                  sas_device_set_phy(found, phy->port);
>                  if (phy->port->num_phys == 0) {
>                          list_add_tail(&phy->port->del_list,
>                                  &parent->port->sas_port_del_list);
>                          if (ex_dev->parent_port == phy->port)
>                                  ex_dev->parent_port = NULL;
>                  }
>                  phy->port = NULL;
>          }
> }

Thanks,
John