Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 19 May 2023 15:42:14 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Jiaqi Yan <jiaqiyan@google.com>, naoya.horiguchi@nec.com
Cc:     songmuchun@bytedance.com, shy828301@gmail.com,
        linmiaohe@huawei.com, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        duenwen@google.com, axelrasmussen@google.com, jthoughton@google.com
Subject: Re: [PATCH v1 1/3] mm/hwpoison: find subpage in hugetlb HWPOISON list
Message-ID: <20230519224214.GB3581@monkey>
References: <20230517160948.811355-1-jiaqiyan@google.com>
 <20230517160948.811355-2-jiaqiyan@google.com>
 <20230517235314.GB10757@monkey>
 <CACw3F52zNguJ-MvXOAJuMK+JfreLxorvHDPwO8w_gQdOzWj7eA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACw3F52zNguJ-MvXOAJuMK+JfreLxorvHDPwO8w_gQdOzWj7eA@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WGNGSStRVzhFU0VVVmtWZlE4UDFodUVKYWVFVkFtWmc3UmhGQm1iVjk1bGUv?=
 =?utf-8?B?eE1yL2F0UzFiZGcrZ3VhWUFSTVRHWCtqK0cxZDRGM3hVZE5RWmlhb0VFUzNu?=
 =?utf-8?B?VDlmSzl6NFZTQTdkcVNhK1BOOWkvSDM3bk8rQzh3VmFZdHk0MUpWUWF1NjBR?=
 =?utf-8?B?WEM4WThJZ2M0MFdPVjBLYk5PUHMzbTJFMkNldmRRQTdkeGlIVEJ1U2J6TTBm?=
 =?utf-8?B?aUF3Vm1vZG90dk1aeHdXRXZpVjlDaEtvOGlMQk8yclUxTEtXNENDL3J2QVRs?=
 =?utf-8?B?NjN2em5tKyt1VFBZOUd5a1Z2L1YwL3ZscGY4OHg2MFBlY2NLUDYrNXdKWkRa?=
 =?utf-8?B?aWpJa01iV0tEMzNjc2c5bzFLUGNadjJuYyt5eVFlRDEzUUNqOE1BcE9kUkNB?=
 =?utf-8?B?ck9WU1ppV2d5eCs0bUg5enpXQzc2SERwaUVyejZsSFpNU2lxd3VxRWFBMWRu?=
 =?utf-8?B?WEJQbXNZeXJ1SGdoTHNDTmdMTEViREpWTHRMRERwcTZoQWRocGlodks0L1pj?=
 =?utf-8?B?TStQNkc2c1VpRC8xSFJ6UlM1VGxORHhqcFgvVmo1SkFjWnRjQVQrdmhQSHg2?=
 =?utf-8?B?UXNLT2tzc3JxbjJYenlZbTdWVStZd21PT1gwZ1Z2MzhRY2FnbksveE1PUEs5?=
 =?utf-8?B?SDh3WU5yVUVnQzBqOExiclRnbEZZcnNnZ1YvU2F0STN2dXJ3SGFuSmZhenRQ?=
 =?utf-8?B?RHlDSzlWb2I3N0d5WW5ZWmQxRmhtd0FsV3hMQmJhdnIrMTh5WGlLZmdTbytv?=
 =?utf-8?B?OFRLYzh2Y2FWWDVzN1B5UVQ0d2NNazN2bjVlODBqWUVxVjZIRCszc244clBL?=
 =?utf-8?B?TTZsUDVHSHBweWwzQnhVRnFDcG1XSW41elVnTWZ2aXREQkdERHQrSTRuaVBB?=
 =?utf-8?B?Y0xQa1dHL05VaGU4WVFXNHQ3bEVNNmlJSzJvNkY5WVVkQnFjNGVjZ2NiVzd5?=
 =?utf-8?B?NW0wUEg4dXFHUnl3MW55d3pSQU80UmFyTnJERkh3VWFzVU5yVnV6d1A1cVpr?=
 =?utf-8?B?UExRMTNUVENuK3FKNlV1UVhwQzU3WmVtQjNvNVVuY2xUcDdSY2ExK0lucG9z?=
 =?utf-8?B?emlKVy9LOHM4aWw4UXlCYVpiRW9wTk5BSjV1T0d1RnpHTnpIOFdkeGxBNFQ1?=
 =?utf-8?B?cUsvYzFkV1Azc1JBTHBsMUsrNjdMUHNGa1RwWW1RdDE3aWhxYmRaTkdlemUy?=
 =?utf-8?B?YU9zeVhMTlJKTTR2VmtYZzRwcE9xWmNNN1RqeGZiNEp3Mk1pdWpLVkNzTXJG?=
 =?utf-8?B?N3pUYmlrRTBKbFZrbVB1WlUweS91UnBMaFBQZTdtN3M1ZEkxdWpZdzhOSlQr?=
 =?utf-8?B?WTFhVmpacGxtbDNjZ2F0UEx3NEZlK3lDUkdrZlU4NEpIZ2Uxc1J1a05MU1pO?=
 =?utf-8?B?aHRFckNmNklVbE8yYVNWWlNzcmFqYk1HYmlLc0U2THhOcEgxRnFYTnM5c0o4?=
 =?utf-8?B?VEpJTWFkRzljRHlZNWV5SnJkWmcwbmt3YjgrUk9HRytQbWY0K1VyZjRkRDNS?=
 =?utf-8?B?UEtaQ1k1dHMxMVEwZlQxV3YweHp2d09vemgyTmQwalVHWWNodWd1elRZbXY5?=
 =?utf-8?B?eXFETlorZ0dITlpVWHc3Y3hmK1pscmVPakw4aGlPUFFhNGhRaTdsd01QSlFZ?=
 =?utf-8?B?ek8xV1lRZVhSUzhvVVB1dGdyREVKWUFFaTY5YStZZ3h5UUYxb0draUhZVFpW?=
 =?utf-8?B?T2xHWDc3VUVTTVd4c3d2UHZGY1h4VXFvNk5BY0U3SFUwWXJyeEo5TlpOZDRP?=
 =?utf-8?B?OS9mdE1XMTBOVVB0NmpoYjRzb3h6aWZjc2FhZUxtTVVUTEVEamZ4V3RZUTdH?=
 =?utf-8?B?aFZvL1M0QVdaVXF3emVLdjE4SW0zNVhxc1ZMSm13emltM1Z5dDlUdFZSLzFv?=
 =?utf-8?B?U01wNjB6RDViNUYvQ3VCUjBGZUJqSDh2cFZvVGEzZkI3bDNPaEdhbW5iN0tK?=
 =?utf-8?B?cGJZckhKOEVLQlZXb1B6VmNPVTVxeTgyeWxlMFJQY1hqSU5JUy8yWCtBVWRm?=
 =?utf-8?B?YnlmZ0ZBYjJ6WDU0M21zUVlyc2I5QzNpeDM3NWExK1pXZ2FweTdpSGNFK3cw?=
 =?utf-8?B?dUxrc2w3OXYxSGtJWDBVRjZZZ1c1M3JvbXdIQTlOUFZTTzRJSXRqRCtmRC9L?=
 =?utf-8?Q?v0+Pe6xno0hsrarh5BoYtwXOU?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?eGVqc0lXaDI3TVRGUDRXMGFEWlBFaGhaU2VEa0hvM3UrVysxdjBteXNKZzMx?=
 =?utf-8?B?c0NZUlBmZmluSHF5OUNVSFNCM3p0M1gzakpHZUJTcWh3NFlUYWZZeC9hNTlW?=
 =?utf-8?B?MWNRYzVPMFFVNmtLb25iVnJKWTBXc0xibDFXZUtNdjVsUGgrNU44eXcydnlW?=
 =?utf-8?B?Z29COWkzUzhKei8vTnRoRmR5eFdBV1I0d2lkSGo0UzBYYnl6N25rZU52RERQ?=
 =?utf-8?B?UGZ6VlVYNHNnb0pVeGdSRDltZWlwUWNOMkxzcXk0d3FWWkhxTnZtSzJ2Q3hh?=
 =?utf-8?B?UEpJeDRTWEdsR2VpT0FvY1dvZWhEZ3VldGRlY3V0NGEzc00xU3M0VXhJWHIw?=
 =?utf-8?B?Q3VIVG5Ud1ByZUJ1QjZDbnRFWEdvQzZJWXpxSXY3eHY2M0ZHaHZwQUM1WExr?=
 =?utf-8?B?Q05ibGJLWkRIWk14YmVaejBqNjNJaWRWc0QyUmcwS3JCcmNTMXczVWI0N3Vo?=
 =?utf-8?B?MTZTWnRRemxwcXdtQUNINHMxOFBuU1M0M0FlTnI4bnFhdnFRMEErd2d3TE5q?=
 =?utf-8?B?M2x4aEVaNFZwTlNHVkRoanJaVG1MU01ZUjdSWENmakR0ZjcxcTNGbm5TOS8r?=
 =?utf-8?B?QUZXWFpWeWlLK3g4NittODJzdHhTSDRiOEJPdTVvdVBoSXczUi9XMG1EZ3pU?=
 =?utf-8?B?QWhlMnJQWG1YOXRuTjFrVm9qeFA3S3QvZzVnVXdHNGlpZG1LNCtJd01INEtp?=
 =?utf-8?B?eExabWlQU1YyeUVJRTdYQnhBamNPd0UyNW81bzZTWFRCUUx6OEw1S2U0aWto?=
 =?utf-8?B?NmJRbmhsbFlEVzNlNVNwTnBzSTRJeXJJZGZuSmEvb0U1RmYvNVZETXprRzJR?=
 =?utf-8?B?aTFPSTZoU3pGdDQrR2JIZktWYlppQ2NWQXk2N0JrUWFnem1PRG1MUmJYZW4x?=
 =?utf-8?B?NmhMN1QxREthK1ZUdTBEeHFRY1ZuMGwwUTRrTjY1OXRCQW82cWdnR0t0UXp0?=
 =?utf-8?B?aWRKK0NVejVGZXlYNXZEYk9ZZ1p2ZXlld2dkSS9tcFpjYURycHpFQm9UYmpL?=
 =?utf-8?B?dCtwZzZUTU5Jb2lvNloxc0hSRmlQbU1TclNTYXhoTUh2Mjh2Yk9CaU0xK3A2?=
 =?utf-8?B?Z3MwRSsvSi8vaHRGS3NYQjFheWNlY2RSQ2hHelZrS2JuOENqRHpVRXNEMlZv?=
 =?utf-8?B?dHY0ZGlFeE5YWktDbC9iRlUwQjJ3aXJVSFp3MmNpRHFETnUzU3BPcmlvWWNZ?=
 =?utf-8?B?V2VCL2JhdVpzQU5LYWFmTXVUdUNmZ0Z3amZaTkF4V2dyNFZ0S0RNREQ4SHFo?=
 =?utf-8?B?Zlhtd2VtamlicXZxWW1neWw1VFMxUEdvdkVrc1JFaWZzTmora1hJcFlyazVB?=
 =?utf-8?Q?FZS4pB+mtp7aI=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 759847ad-577c-40e8-8b35-08db58ba4c4d
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2023 22:42:17.2683
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fg5zoBsdSS3otfaCfcyO1z2df6wOh6K7Yas8Z6HFbAHnxMGOtq7cOF2EbdOevT2ZEGlxqNJVnYZH81n0sbXU7g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5737
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-19_16,2023-05-17_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0
 mlxlogscore=999 malwarescore=0 bulkscore=0 adultscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2304280000 definitions=main-2305190194
X-Proofpoint-GUID: kFOEPlRMR5W24dwXot41KEyQqadMqNXo
X-Proofpoint-ORIG-GUID: kFOEPlRMR5W24dwXot41KEyQqadMqNXo
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 05/19/23 13:54, Jiaqi Yan wrote:
> On Wed, May 17, 2023 at 4:53 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
> >
> > On 05/17/23 16:09, Jiaqi Yan wrote:
> > > Adds the functionality to search a subpage's corresponding raw_hwp_page
> > > in hugetlb page's HWPOISON list. This functionality can also tell if a
> > > subpage is a raw HWPOISON page.
> > >
> > > Exports this functionality to be immediately used in the read operation
> > > for hugetlbfs.
> > >
> > > Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> > > ---
> > >  include/linux/mm.h  | 23 +++++++++++++++++++++++
> > >  mm/memory-failure.c | 26 ++++++++++++++++----------
> > >  2 files changed, 39 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 27ce77080c79..f191a4119719 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> >
> > Any reason why you decided to add the following to linux/mm.h instead of
> > linux/hugetlb.h?  Since it is hugetlb specific I would have thought
> > hugetlb.h was more appropriate.
> >
> > > @@ -3683,6 +3683,29 @@ enum mf_action_page_type {
> > >   */
> > >  extern const struct attribute_group memory_failure_attr_group;
> > >
> > > +#ifdef CONFIG_HUGETLB_PAGE
> > > +/*
> > > + * Struct raw_hwp_page represents information about "raw error page",
> > > + * constructing singly linked list from ->_hugetlb_hwpoison field of folio.
> > > + */
> > > +struct raw_hwp_page {
> > > +     struct llist_node node;
> > > +     struct page *page;
> > > +};
> > > +
> > > +static inline struct llist_head *raw_hwp_list_head(struct folio *folio)
> > > +{
> > > +     return (struct llist_head *)&folio->_hugetlb_hwpoison;
> > > +}
> > > +
> > > +/*
> > > + * Given @subpage, a raw page in a hugepage, find its location in @folio's
> > > + * _hugetlb_hwpoison list. Return NULL if @subpage is not in the list.
> > > + */
> > > +struct raw_hwp_page *find_raw_hwp_page(struct folio *folio,
> > > +                                    struct page *subpage);
> > > +#endif
> > > +
> > >  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_HUGETLBFS)
> > >  extern void clear_huge_page(struct page *page,
> > >                           unsigned long addr_hint,
> > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > > index 5b663eca1f29..c49e6c2d1f07 100644
> > > --- a/mm/memory-failure.c
> > > +++ b/mm/memory-failure.c
> > > @@ -1818,18 +1818,24 @@ EXPORT_SYMBOL_GPL(mf_dax_kill_procs);
> > >  #endif /* CONFIG_FS_DAX */
> > >
> > >  #ifdef CONFIG_HUGETLB_PAGE
> > > -/*
> > > - * Struct raw_hwp_page represents information about "raw error page",
> > > - * constructing singly linked list from ->_hugetlb_hwpoison field of folio.
> > > - */
> > > -struct raw_hwp_page {
> > > -     struct llist_node node;
> > > -     struct page *page;
> > > -};
> > >
> > > -static inline struct llist_head *raw_hwp_list_head(struct folio *folio)
> > > +struct raw_hwp_page *find_raw_hwp_page(struct folio *folio,
> > > +                                    struct page *subpage)
> > >  {
> > > -     return (struct llist_head *)&folio->_hugetlb_hwpoison;
> > > +     struct llist_node *t, *tnode;
> > > +     struct llist_head *raw_hwp_head = raw_hwp_list_head(folio);
> > > +     struct raw_hwp_page *hwp_page = NULL;
> > > +     struct raw_hwp_page *p;
> > > +
> > > +     llist_for_each_safe(tnode, t, raw_hwp_head->first) {
> >
> > IIUC, in rare error cases a hugetlb page can be poisoned WITHOUT a
> > raw_hwp_list.  This is indicated by the hugetlb page specific flag
> > RawHwpUnreliable or folio_test_hugetlb_raw_hwp_unreliable().
> >
> > Looks like this routine does not consider that case.  Seems like it should
> > always return the passed subpage if folio_test_hugetlb_raw_hwp_unreliable()
> > is true?
> 
> Thanks for catching this. I wonder should this routine consider
> RawHwpUnreliable or should the caller do.
> 
> find_raw_hwp_page now returns raw_hwp_page* in the llist entry to
> caller (valid one at the moment), but once RawHwpUnreliable is set,
> all the raw_hwp_page in the llist will be kfree(), and the returned
> value becomes dangling pointer to caller (if the caller holds that
> caller long enough). Maybe returning a bool would be safer to the
> caller? If the routine returns bool, then checking RawHwpUnreliable
> can definitely be within the routine.

I think the check for RawHwpUnreliable should be within this routine.
Looking closer at the code, I do not see any way to synchronize this.
It looks like manipulation in the memory-failure code would be
synchronized via the mf_mutex.  However, I do not see how traversal and
freeing of the raw_hwp_list  called from __update_and_free_hugetlb_folio
is synchronized against memory-failure code modifying the list.

Naoya, can you provide some thoughts?

> 
> Another option is, this routine simply doesn one thing: find a
> raw_hwp_page in raw_hwp_list for a subpage. But the caller needs to 1)
> test RawHwpUnreliable before calls into the routine, and 2) test
> RawHwpUnreliable before access returned raw_hwp_page*. I think 2nd
> option will be error-prone and the 1st option is a better one.
> 
> Maybe I am over-thinking. What do you think?

I think racing code accessing the raw_hwp_list is very unlikely.
However, it is possible and should be considered.
-- 
Mike Kravetz

> 
> > --
> > Mike Kravetz
> >
> > > +             p = container_of(tnode, struct raw_hwp_page, node);
> > > +             if (subpage == p->page) {
> > > +                     hwp_page = p;
> > > +                     break;
> > > +             }
> > > +     }
> > > +
> > > +     return hwp_page;
> > >  }
> > >
> > >  static unsigned long __folio_free_raw_hwp(struct folio *folio, bool move_flag)
> > > --
> > > 2.40.1.606.ga4b1b128d6-goog
> > >